Re: [qemu-s390x] [Qemu-devel] [PATCH v1 01/15] s390x/tcg: Fix TEST DATA CLASS instructions

[David Hildenbrand]

On 12.02.19 19:01, Richard Henderson wrote:
> On 2/12/19 3:02 AM, David Hildenbrand wrote:
>> +static bool s390_tdc32(CPUS390XState *env, float32 f1, uint16_t dc_mask)
>> +{
>> +        const bool neg = float32_is_neg(f1);
>> +        const bool zero = float32_is_zero(f1);
>> +        const bool normal = float32_is_normal(f1);
>> +        const bool denormal = float32_is_denormal(f1);
>> +        const bool infinity = float32_is_infinity(f1);
>> +        const bool quiet_nan = float32_is_quiet_nan(f1, &env->fpu_status);
>> +        const bool sig_nan = float32_is_signaling_nan(f1, &env->fpu_status);
>> +
>> +        return (zero && test_dc_mask(dc_mask, 0, neg)) ||
>> +               (normal && test_dc_mask(dc_mask, 2, neg)) ||
>> +               (denormal && test_dc_mask(dc_mask, 4, neg)) ||
>> +               (infinity && test_dc_mask(dc_mask, 6, neg)) ||
>> +               (quiet_nan && test_dc_mask(dc_mask, 8, neg)) ||
>> +               (sig_nan && test_dc_mask(dc_mask, 10, neg));
>> +}
>> +
> 
> This is doing more work than necessary, since any one fp value can only be one
> of these.
> 
> I think it would be better to structure this like the riscv helper_fclass_*:
> 
> static inline uint32_t dcmask(int bit, bool neg)
> {
>     return 1 << (11 - bit - neg);
> }
> 
> static uint32_t float32_dcmask(CPUS390XState *env, float32 f1)
> {
>     bool neg = float32_is_neg(f1);
> 
>     /* Sorted by most common cases.  */
>     if (float32_is_normal(f1)) {
>         return dc_mask(2, neg);
>     } else if (float32_is_zero(f1)) {
>         return dc_mask(0, neg);
>     } else if (float32_is_zero_or_denormal(f1)) {
>         /* denormal, since zero is eliminated */
>         return dc_mask(4, neg);
>     } else if (float32_is_infinity(f1)) {
>         return dc_mask(6, neg);
>     } else if (float64_is_quiet_nan(f1, &env->fpu_status)) {
>         return dc_mask(8, neg);
>     } else {
>         /* signaling nan, as last remaining case */
>         return dc_mask(10, neg);
>     }
> }
> 
> uint32_t HELPER(tceb)(CPUS390XState *env, uint64_t f1, uint64_t m2)
> {
>     return (m2 & float32_dcmask(env, f1)) != 0;
> }
> 
> You may or may not wish to macro-ise float32_dcmask for the float type.
> 
> 
> r~
> 

Indeed, makes sense! Also thought about macros, but I guess the huge
pile of code for vector instruction support I have lying around make me
lazy :)

Thanks!

-- 

Thanks,

David / dhildenb