Re: [RFC PATCH 1/3] target/ppc: Bugfix fadd/fsub result with OE/UE set

[Lucas Mateus Martins Araujo e Castro <lucas.araujo@eldorado.org.br>]

[-- Attachment #1: Type: text/plain, Size: 5393 bytes --]


On 03/08/2022 13:18, Richard Henderson wrote:
>
> On 8/3/22 05:22, Lucas Mateus Castro(alqotel) wrote:
>> From: "Lucas Mateus Castro (alqotel)" <lucas.araujo@eldorado.org.br>
>>
>> As mentioned in the functions float_overflow_excp and
>> float_underflow_excp, the result should be adjusted as mentioned in the
>> ISA (subtracted 192/1536 from the exponent of the intermediate result if
>> an overflow occurs with OE set and added 192/1536 to the exponent of the
>> intermediate result if an underflow occurs with UE set), but at those
>> functions the result has already been rounded so it is not possible to
>> add/subtract from the intermediate result anymore.
>>
>> This patch creates a new function that receives the value that should be
>> subtracted/added from the exponent if an overflow/underflow happens, to
>> not leave some arbitrary numbers from the PowerISA in the middle of the
>> FPU code. If these numbers are 0 the new functions just call the old
>> ones.
>>
>> I used 2 values here for overflow and underflow, maybe it'd be better to
>> just use the same ones, any thoughts?
>>
>> Signed-off-by: Lucas Mateus Castro (alqotel) 
>> <lucas.araujo@eldorado.org.br>
>> ---
>> An alternative I've thought was to always return the value adjusted if a
>> overflow or underflow occurs and in float_underflow_excp and
>> float_overflow_excp adjust it to inf/den/0 if OE/UE is 0, but I didn't
>> saw many advantages to that approach.
>> ---
>>   fpu/softfloat.c         | 75 +++++++++++++++++++++++++++++++++++++++++
>>   include/fpu/softfloat.h |  2 ++
>>   target/ppc/fpu_helper.c | 10 ++++--
>>   3 files changed, 85 insertions(+), 2 deletions(-)
>>
>> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
>> index 4a871ef2a1..a407129dcb 100644
>> --- a/fpu/softfloat.c
>> +++ b/fpu/softfloat.c
>> @@ -268,6 +268,8 @@ typedef bool (*f64_check_fn)(union_float64 a, 
>> union_float64 b);
>>
>>   typedef float32 (*soft_f32_op2_fn)(float32 a, float32 b, 
>> float_status *s);
>>   typedef float64 (*soft_f64_op2_fn)(float64 a, float64 b, 
>> float_status *s);
>> +typedef float64 (*soft_f64_op2_int2_fn)(float64 a, float64 b, int c, 
>> int d,
>> +                                        float_status *s);
>>   typedef float   (*hard_f32_op2_fn)(float a, float b);
>>   typedef double  (*hard_f64_op2_fn)(double a, double b);
>>
>> @@ -401,6 +403,19 @@ float64_gen2(float64 xa, float64 xb, 
>> float_status *s,
>>       return soft(ua.s, ub.s, s);
>>   }
>>
>> +static inline float64
>> +float64_gen2_excp(float64 xa, float64 xb, int xc, int xd, 
>> float_status *s,
>> +                  hard_f64_op2_fn hard, soft_f64_op2_fn soft,
>> +                  soft_f64_op2_int2_fn soft_excp, f64_check_fn pre,
>> +                  f64_check_fn post)
>> +{
>> +    if (xc || xd) {
>> +        return soft_excp(xa, xb, xc, xd, s);
>> +    } else {
>> +        return float64_gen2(xa, xb, s, hard, soft, pre, post);
>> +    }
>> +}
>> +
>>   /*
>>    * Classify a floating point number. Everything above float_class_qnan
>>    * is a NaN so cls >= float_class_qnan is any NaN.
>> @@ -1929,6 +1944,39 @@ static double hard_f64_sub(double a, double b)
>>       return a - b;
>>   }
>>
>> +static float64 QEMU_SOFTFLOAT_ATTR
>> +soft_f64_addsub_excp_en(float64 a, float64 b, int oe_sub, int ue_sum,
>> +                        float_status *status, bool subtract)
>> +{
>> +    FloatParts64 pa, pb, *pr;
>> +
>> +    float64_unpack_canonical(&pa, a, status);
>> +    float64_unpack_canonical(&pb, b, status);
>> +    pr = parts_addsub(&pa, &pb, status, subtract);
>> +
>> +    if (unlikely(oe_sub && (pr->exp > 1023))) {
>> +        pr->exp -= oe_sub;
>> +        float_raise(float_flag_overflow, status);
>> +    } else if (unlikely(ue_sum && (pr->exp < -1022))) {
>> +        pr->exp += ue_sum;
>> +        float_raise(float_flag_underflow, status);
>> +    }
>> +
>> +    return float64_round_pack_canonical(pr, status);
>
> This is incorrect, because the exponent is not fixed until the middle 
> of round_pack_canonical.
>
> I think you should not add new functions like this, with new 
> parameters, but instead add
> fields to float_status, which would then be checked at the places 
> currently setting
> underflow and overflow.

So add overflow_correction and underflow_correction in 
'partsN(uncanon_normal)' so that:

if (exp >= exp_max) {
     if (overflow_correction != 0) {
         exp -= overflow_correction;
     }
}

And the equivalent for underflow, or a bool ppc_overflow_enable that 
uses a fixed value like:

if (exp >= exp_max) {
     if (ppc_overflow_enable) {
         exp -= ((fmt->exp_bias + 1) + (fmt->exp_bias + 1)/2);
     }
}

(and the equivalent for underflow) ?

>
>
> r~
-- 
Lucas Mateus M. Araujo e Castro
Instituto de Pesquisas ELDORADO 
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

[-- Attachment #2: Type: text/html, Size: 8301 bytes --]