Re: [PATCH v6 26/61] target/riscv: vector single-width fractional multiply with rounding and saturation

[LIU Zhiwei]

On 2020/3/28 9:08, Richard Henderson wrote:
> On 3/17/20 8:06 AM, LIU Zhiwei wrote:
> > +static int64_t vsmul64(CPURISCVState *env, int vxrm, int64_t a, int64_t b)
> > +{
> > +    uint8_t round;
> > +    uint64_t hi_64, lo_64, Hi62;
> > +    uint8_t hi62, hi63, lo63;
> > +
> > +    muls64(&lo_64, &hi_64, a, b);
> > +    hi62 = extract64(hi_64, 62, 1);
> > +    lo63 = extract64(lo_64, 63, 1);
> > +    hi63 = extract64(hi_64, 63, 1);
> > +    Hi62 = extract64(hi_64, 0, 62);
> This seems like way more work than necessary.
>
> > +    if (hi62 != hi63) {
> > +        env->vxsat = 0x1;
> > +        return INT64_MAX;
> > +    }
> This can only happen for a == b == INT64_MIN.
> Perhaps just test exactly that and move it above the multiply?
>
> > +    round = get_round(vxrm, lo_64, 63);
> > +    if (round && (Hi62 == 0x3fffffff) && lo63) {
> > +        env->vxsat = 0x1;
> > +        return hi62 ? INT64_MIN : INT64_MAX;
> > +    } else {
> > +        if (lo63 && round) {
> > +            return (hi_64 + 1) << 1;
> > +        } else {
> > +            return (hi_64 << 1) | lo63 | round;
> > +        }
> > +    }
Yes, it's an error here. As INT64_MIN will never be  a right result.
>    /* Cannot overflow, as there are always
>       2 sign bits after multiply. */
>    ret = (hi_64 << 1) | (lo_64 >> 63);
>    if (round) {
>        if (ret == INT64_MAX) {
>            env->vxsat = 1;
>        } else {
>            ret += 1;
>        }
>    }
>    return ret;
Thanks. I think it's  right and much clearer.

Zhiwei.
> r~