Re: [Qemu-devel] [PATCH 14/17] s390x: Implement opcode helpers

[Peter Maydell]

On 28 March 2011 18:23, Alexander Graf <address@hidden> wrote:
> On 03/24/2011 06:29 PM, Peter Maydell wrote:
>>> +/* condition codes for binary FP ops */
>>> +static uint32_t set_cc_f32(float32 v1, float32 v2)
>>> +{
>>> +    if (float32_is_any_nan(v1) || float32_is_any_nan(v2)) {
>>> +        return 3;
>>> +    } else if (float32_eq(v1, v2,&env->fpu_status)) {
>>> +        return 0;
>>> +    } else if (float32_lt(v1, v2,&env->fpu_status)) {
>>> +        return 1;
>>> +    } else {
>>> +        return 2;
>>> +    }
>>> +}
>>
>> Can you not use float32_compare_quiet() (returns a value
>> telling you if it's less/equal/greater/unordered)?
>> If not, needs a comment saying why you need to do it the hard way.
>
> I just checked the macros there and it looks like float32_compare_quiet
> returns eq when both numbers are NaN.

Hmm?

    if (( ( extractFloat ## s ## Exp( a ) == nan_exp ) &&                    \
         extractFloat ## s ## Frac( a ) ) ||                                 \
        ( ( extractFloat ## s ## Exp( b ) == nan_exp ) &&                    \
          extractFloat ## s ## Frac( b ) )) {                                \
        if (!is_quiet ||                                                     \
            float ## s ## _is_signaling_nan( a ) ||                          \
            float ## s ## _is_signaling_nan( b ) ) {                         \
            float_raise( float_flag_invalid STATUS_VAR);                     \
        }                                                                    \
        return float_relation_unordered;                                     \
    }                                                                        \

If A is a NaN (ie its exponent is nan_exp and the frac bits aren't zero)
or B is a NaN then we return float_relation_unordered.

> We would still have to convert from
> the return value from that over to a CC value. I honestly don't see any
> benefit - the code doesn't become cleaner or smaller.

So you get
static uint32_t set_cc_f32(float32 v1, float32 v2)
{
    switch (float32_compare_quiet(v1, v2, &env->fpu_status)) {
    case float_relation_unordered:
        return 3;
    case float_relation_equal:
        return 0;
    case float_relation_less:
        return 1;
    case float_relation_greater:
        return 2;
    case float_relation_unordered:
        return 3;
    }
}

(and you probably want to put the conversion switch into a function
since you'll be using it several times.)

Which I think is pretty straightforward, cleaner because we only
call one softfloat function rather than several, and should be
faster too (we get to avoid repeating a pile of tedious bit manipulation
in the eq and lt functions).

>>> +/* load 128-bit FP zero */
>>> +void HELPER(lzxr)(uint32_t f1)
>>> +{
>>> +    CPU_QuadU x;
>>> +    x.q = float64_to_float128(float64_zero,&env->fpu_status);
>>
>> Yuck. Just define a float128_zero if we need one.
>
> Good point. Mind to do so? I find myself struggling with the code there.

We could just follow the pattern of  float128_default_nan_{low,high}
in softfloat.h:

#define float128_zero_low LIT64(0)
#define float128_zero_high LIT64(0)

then your function has:
 x.q.high = float128_zero_high;
 x.q.low = float128_zero_low;

Or we could do something with an expression that returns a
struct type; that would be cleaner. I think the default nan
code is assuming it might have to be compiled with something
other than gcc. However I forget the C syntax and have to go
home now :-)

-- PMM