Re: [RFC v4 14/70] target/riscv: rvv-1.0: update check functions

[Richard Henderson <richard.henderson@linaro.org>]

On 8/17/20 1:48 AM, frank.chang@sifive.com wrote:
> +static inline bool is_aligned(const uint8_t val, const uint8_t pos)
> +{
> +    return pos ? (val & (pos - 1)) == 0 : true;
> +}

The truncation to uint8_t from int is odd.  Can we drop all of that and just
use int?

Looking at the uses, I think that you should pass lmul directly instead of
requiring the callers to all compute 1 << lmul, and also verify that lmul is
positive.

That change makes this function look like

    return lmul <= 0 || extract32(val, 0, lmul) == 0;


> +static inline bool is_overlapped(const uint8_t astart, uint8_t asize,
> +                                 const uint8_t bstart, uint8_t bsize)
> +{
> +    asize = asize == 0 ? 1 : asize;
> +    bsize = bsize == 0 ? 1 : bsize;

This looks odd.  Again, I think passing in lmul would be better than size.
Then compute size here locally:

    int asize = amul <= 0 ? 1 : 1 << amul;

> +
> +    const int aend = astart + asize;
> +    const int bend = bstart + bsize;
> +
> +    return MAX(aend, bend) - MIN(astart, bstart) < asize + bsize;
> +}
> +
> +static inline bool is_overlapped_widen(const uint8_t astart, uint8_t asize,
> +                                       const uint8_t bstart, uint8_t bsize)

This needs more comments, I think.  It's not obvious why this is (or needs to
be) different from is_overlapped.

I think you're trying to implement the

  * destination eew smaller than source eew,
    and overlap is allowed at the beginning.
  * destination eew larger than source eew,
    and overlap is allowed at the end.

rule from section 5.2.  But since you're not comparing asize vs bsize, that's
not what you're doing.

Anyway, I think all of these rules can be put into require_noover, and there
need not be a separate require_noover_widen.

> +static bool require_rvv(DisasContext *s)
> +{
> +    if (s->mstatus_vs == 0) {
> +        return false;
> +    }
> +    return true;

    return s->mstatus_vs != 0;

> +static bool vext_check_sss(DisasContext *s, int vd, int vs1,
> +                           int vs2, int vm, bool is_vs1)
> +{
> +    bool ret = require_vm(vm, vd);
> +    if (s->lmul > 0) {
> +        ret &= require_align(vd, 1 << s->lmul) &&
> +               require_align(vs2, 1 << s->lmul);
> +        if (is_vs1) {
> +            ret &= require_align(vs1, 1 << s->lmul);
> +        }
> +    }
> +    return ret;
> +}

I think this (and similar function taking is_vs1) should be split.  All callers
pass a constant value, and thus can just as easily call a different function.

Perhaps

static bool vext_check_ss(DisasContext *s, int vd,
                          int vs2, int vm)
{
    return (require_vm(vm, vd) &&
            require_align(vd, s->lmul) &&
            require_align(vs2, s->lmul));
}

static bool vext_check_sss(DisasContext *s, int vd, int vs1,
                           int vs2, int vm)
{
    return (vext_check_ss(s, vd, vs2, vm) &&
            require_align(vs1, s->lmul));
}

> +/*
> + * Check function for maskable vector instruction with format:
> + * single-width result and single-width sources (SEW = SEW op SEW)
> + *
> + * is_vs1: indicates whether insn[19:15] is a vs1 field or not.
> + *
> + * Rules to be checked here:
> + *   1. Source (vs2, vs1) vector register number are multiples of LMUL.
> + *      (Section 3.3.2)
> + *   2. Destination vector register cannot overlap a source vector
> + *      register (vs2, vs1) group.
> + *      (Section 5.2)
> + */
> +static bool vext_check_mss(DisasContext *s, int vd, int vs1,
> +                           int vs2, bool is_vs1)
>  {
> +    bool ret = require_align(vs2, 1 << s->lmul);
> +    if (vd != vs2) {
> +        ret &= require_noover(vd, 1, vs2, 1 << s->lmul);
> +    }
> +    if (is_vs1) {
> +        if (vd != vs1) {
> +            ret &= require_noover(vd, 1, vs1, 1 << s->lmul);
> +        }
> +        ret &= require_align(vs1, 1 << s->lmul);
> +    }
> +    return ret;
> +}

If require_noover implements all of the overlap rules, as suggested, this
simplifies to

static bool vext_check_ms(DisasContext *s, int vd, int vs2)
{
    return (require_align(vs2, s->lmul) &&
            require_noover(vd, 0, vs2, s->lmul);
}

static bool vext_check_mss(DisasContext *s, int vd,
                           int vs1, int vs2)
{
    return (vext_check_ms(s, vd, vs2) &&
            require_align(vs1, s->lmul) &&
            require_noover(vd, 0, vs1, s->lmul));
}

> +/*
> + * Common check function for vector widening instructions
> + * of double-width result (2*SEW).
> + *
> + * Rules to be checked here:
> + *   1. The largest vector register group used by an instruction
> + *      can not be greater than 8 vector registers (Section 5.2):
> + *      => LMUL < 8.
> + *      => SEW < 64.
> + *   2. Destination vector register number is multiples of 2 * LMUL.
> + *      (Section 3.3.2, 11.2)
> + *   3. Destination vector register group for a masked vector
> + *      instruction cannot overlap the source mask register (v0).
> + *      (Section 5.3)
> + */
> +static bool vext_wide_check_common(DisasContext *s, int vd, int vm)
> +{
> +    return (s->lmul <= 2) &&
> +           (s->sew < 3) &&

Use MO_64 here for clarity.

> +static bool vext_narrow_check_common(DisasContext *s, int vd, int vs2,
> +                                     int vm)
> +{
> +    return (s->lmul <= 2) &&
> +           (s->sew < 3) &&

Likewise.

> +/*
> + * Check function for vector instruction with format:
> + * double-width result and single-width sources (2*SEW = SEW op SEW)
>   *
> + * is_vs1: indicates whether insn[19:15] is a vs1 field or not.
>   *
> + * Rules to be checked here:
> + *   1. All rules in defined in widen common rules are applied.
> + *   2. Source (vs2, vs1) vector register number are multiples of LMUL.
> + *      (Section 3.3.2)
> + *   3. Destination vector register cannot overlap a source vector
> + *      register (vs2, vs1) group.
> + *      (Section 5.2)
>   */
> +static bool vext_check_dss(DisasContext *s, int vd, int vs1, int vs2,
> +                           int vm, bool is_vs1)
>  {
> +    bool ret = (vext_wide_check_common(s, vd, vm) &&
> +                require_align(vs2, 1 << s->lmul));
> +    if (s->lmul < 0) {
> +        ret &= require_noover(vd, 1 << (s->lmul + 1), vs2, 1 << s->lmul);
> +    } else {
> +        ret &= require_noover_widen(vd, 1 << (s->lmul + 1), vs2, 1 << s->lmul);
> +    }

This is buggy, with (1 << negative_number), and is exactly why I think
require_noover needs to be passed the emul of each operand and implement all of
the rules.

This should just be

static bool vext_check_ds(DisasContext *s, int vd, int vs2)
{
    return (vext_wide_check_common(s, vd, vm) &&
            require_align(vs2, s->lmul) &&
            require_noover(vd, s->lmul + 1, vs2, s->lmul));
}

static bool vext_check_dss(DisasContext *s, int vd,
                           int vs1, int vs2)
{
    return (vext_check_ds(s, vd, vs2) &&
            require_align(vs1, s->lmul) &&
            require_noover(vd, s->lmul + 1, vs1, s->lmul));
}

static bool vext_check_dds(DisasContext *s, int vd,
                           int vs1, int vs2)
{
    return (vext_check_ds(s, vd, vs1) &&
            require_align(vs2, s->lmul + 1) &&
            require_noover(vd, s->lmul + 1, vs1, s->lmul + 1));
}

>  /*
> + * Check function for vector reduction instructions.
> + *
> + * Rules to be checked here:
> + *   1. Source 1 (vs2) vector register number is multiples of LMUL.
> + *      (Section 3.3.2)
> + *   2. For widening reduction instructions, SEW < 64.
> + *
> + * TODO: Check vstart == 0
>   */
> +static bool vext_check_reduction(DisasContext *s, int vs2, bool is_wide)
>  {
> +    bool ret = require_align(vs2, 1 << s->lmul);
> +    if (is_wide) {
> +        ret &= s->sew < 3;
> +    }
> +    return ret;
>  }

Again, should be split.  But in this case probably into the only callers...

> +static bool reduction_widen_check(DisasContext *s, arg_rmrr *a)
> +{
> +    return require_rvv(s) &&
> +           vext_check_isa_ill(s) &&
> +           vext_check_reduction(s, a->rs2, true);
> +}

This could simplify to

    return reduction_check(s, a) && s->sew < MO_64;


r~