Re: [Qemu-devel] [PATCH 4/5] tricore: add QSEED instruction

[Konopik, Andreas (EFS-GH2)]

Hi Bastian,

> Hi,
> 
> On 6/5/19 8:11 AM, David Brenken wrote:
> > +/*
> > + * Target TriCore QSEED.F significand Lookup Table
> > + *
> > + * The QSEED.F output significand depends on the least-significant
> > + * exponent bit and the 6 most-significant significand bits.
> > + *
> > + * IEEE 754 float datatype
> > + * partitioned into Sign (S), Exponent (E) and Significand (M):
> > + *
> > + * S   E E E E E E E E   M M M M M M ...
> > + *    |             |               |
> > + *    +------+------+-------+-------+
> > + *           |              |
> > + *          for        lookup table
> > + *      calculating     index for
> > + *        output E       output M
> > + */
> > +static const uint8_t target_qseed_significand_table[128] = {
> > +    253, 252, 245, 244, 239, 238, 231, 230, 225, 224, 217, 216,
> > +    211, 210, 205, 204, 201, 200, 195, 194, 189, 188, 185, 184,
> > +    179, 178, 175, 174, 169, 168, 165, 164, 161, 160, 157, 156,
> > +    153, 152, 149, 148, 145, 144, 141, 140, 137, 136, 133, 132,
> > +    131, 130, 127, 126, 123, 122, 121, 120, 117, 116, 115, 114,
> > +    111, 110, 109, 108, 103, 102, 99, 98, 93, 92, 89, 88, 83,
> > +    82, 79, 78, 75, 74, 71, 70, 67, 66, 63, 62, 59, 58, 55,
> > +    54, 53, 52, 49, 48, 45, 44, 43, 42, 39, 38, 37, 36, 33,
> > +    32, 31, 30, 27, 26, 25, 24, 23, 22, 19, 18, 17, 16, 15,
> > +    14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2 };
> 
> 
> Can you explain in a comment how you arrived at this lookup table?

We extracted this lookup table from real hardware results.
We also validated QEMU output values by comparing again with the real
hardware qseed output.
Testing was not made for all 2^32 possible input values,but for all
positive floats that yield different outputs (with a big stride).

We will add a shorter comment to the second version of the patchset

> > +    } else if (float32_is_neg(arg1)) {
> > +        result = float32_sqrt_nan;
> > +        env->FPU_FI = 1;
> [...]
> > +
> > +    flags = f_get_excp_flags(env);
> > +    if (flags) {
> > +        if (flags & float_flag_invalid) {
> > +            f_update_psw_flags(env, flags);
> > +        } else {
> > +            env->FPU_FS = 0;
> > +        }
> > +    } else {
> > +        env->FPU_FS = 0;
> 
> You are setting FPU_FS to 0, even though FPU_FI might have been set in case of
> a NaN. I think it's best to remove the whole softfloat check, as none of the
> softfloat functions you call, can raise any flags.

I will make the PSW update only dependent on is_NaN(input) and 
is_SQRT_NaN(output).

Best regards,

Andreas