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

[Alexander Graf]

On 28.03.2011, at 19:55, Peter Maydell wrote:

> 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).

Alrighty, changed the code :).

> 
>>>> +/* 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 :-)

I'll just leave it to you for a follow-up patch :). The less I have to touch in 
softfpu code, the better :)


Alex