diff mbox series

[10/11] softfloat: Sink frac_cmp in parts_pick_nan until needed

Message ID 20241203203949.483774-11-richard.henderson@linaro.org
State Superseded
Headers show
Series fpu: pickNaN follow ups | expand

Commit Message

Richard Henderson Dec. 3, 2024, 8:39 p.m. UTC 
Move the fractional comparison to the end of the
float_2nan_prop_x87 case.  This is not required for
any other 2nan propagation rule.  Reorganize the
x87 case itself to break out of the switch when the
fractional comparison is not required.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 fpu/softfloat-parts.c.inc | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

Comments

Peter Maydell Dec. 5, 2024, 1:48 p.m. UTC | #1 
On Tue, 3 Dec 2024 at 20:40, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Move the fractional comparison to the end of the
> float_2nan_prop_x87 case.  This is not required for
> any other 2nan propagation rule.  Reorganize the
> x87 case itself to break out of the switch when the
> fractional comparison is not required.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

> @@ -89,20 +84,24 @@ static FloatPartsN *partsN(pick_nan)(FloatPartsN *a, FloatPartsN *b,
>           * return the NaN with the positive sign bit (if any).
>           */
>          if (is_snan(a->cls)) {
> -            if (is_snan(b->cls)) {
> -                which = cmp > 0 ? 0 : 1;
> -            } else {
> +            if (!is_snan(b->cls)) {
>                  which = is_qnan(b->cls) ? 1 : 0;
> +                break;
>              }
>          } else if (is_qnan(a->cls)) {
>              if (is_snan(b->cls) || !is_qnan(b->cls)) {

Pre-existing code, but isn't
   is_snan(X) || !is_qnan(X)
the same as
   !is_qnan(X)
?

thanks
-- PMM
Richard Henderson Dec. 5, 2024, 5:51 p.m. UTC | #2 
On 12/5/24 07:48, Peter Maydell wrote:
> On Tue, 3 Dec 2024 at 20:40, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Move the fractional comparison to the end of the
>> float_2nan_prop_x87 case.  This is not required for
>> any other 2nan propagation rule.  Reorganize the
>> x87 case itself to break out of the switch when the
>> fractional comparison is not required.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
>> @@ -89,20 +84,24 @@ static FloatPartsN *partsN(pick_nan)(FloatPartsN *a, FloatPartsN *b,
>>            * return the NaN with the positive sign bit (if any).
>>            */
>>           if (is_snan(a->cls)) {
>> -            if (is_snan(b->cls)) {
>> -                which = cmp > 0 ? 0 : 1;
>> -            } else {
>> +            if (!is_snan(b->cls)) {
>>                   which = is_qnan(b->cls) ? 1 : 0;
>> +                break;
>>               }
>>           } else if (is_qnan(a->cls)) {
>>               if (is_snan(b->cls) || !is_qnan(b->cls)) {
> 
> Pre-existing code, but isn't
>     is_snan(X) || !is_qnan(X)
> the same as
>     !is_qnan(X)
> ?

No, since X may not be a NaN at all.  We arrive in pick_nan knowing only that one operand 
must be a NaN, but the other may be anything at all.


r~
Richard Henderson Dec. 5, 2024, 5:52 p.m. UTC | #3 
On 12/5/24 11:51, Richard Henderson wrote:
> On 12/5/24 07:48, Peter Maydell wrote:
>> On Tue, 3 Dec 2024 at 20:40, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> Move the fractional comparison to the end of the
>>> float_2nan_prop_x87 case.  This is not required for
>>> any other 2nan propagation rule.  Reorganize the
>>> x87 case itself to break out of the switch when the
>>> fractional comparison is not required.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>
>>> @@ -89,20 +84,24 @@ static FloatPartsN *partsN(pick_nan)(FloatPartsN *a, FloatPartsN *b,
>>>            * return the NaN with the positive sign bit (if any).
>>>            */
>>>           if (is_snan(a->cls)) {
>>> -            if (is_snan(b->cls)) {
>>> -                which = cmp > 0 ? 0 : 1;
>>> -            } else {
>>> +            if (!is_snan(b->cls)) {
>>>                   which = is_qnan(b->cls) ? 1 : 0;
>>> +                break;
>>>               }
>>>           } else if (is_qnan(a->cls)) {
>>>               if (is_snan(b->cls) || !is_qnan(b->cls)) {
>>
>> Pre-existing code, but isn't
>>     is_snan(X) || !is_qnan(X)
>> the same as
>>     !is_qnan(X)
>> ?
> 
> No, since X may not be a NaN at all.  We arrive in pick_nan knowing only that one operand 
> must be a NaN, but the other may be anything at all.

Alternately, and more, correctly, you're right.  :-}


r~
diff mbox series

Patch

diff --git a/fpu/softfloat-parts.c.inc b/fpu/softfloat-parts.c.inc
index 099f1c48ef..9a2287095c 100644
--- a/fpu/softfloat-parts.c.inc
+++ b/fpu/softfloat-parts.c.inc
@@ -52,11 +52,6 @@  static FloatPartsN *partsN(pick_nan)(FloatPartsN *a, FloatPartsN *b,
         return a;
     }
 
-    cmp = frac_cmp(a, b);
-    if (cmp == 0) {
-        cmp = a->sign < b->sign;
-    }
-
     switch (s->float_2nan_prop_rule) {
     case float_2nan_prop_s_ab:
         if (have_snan) {
@@ -89,20 +84,24 @@  static FloatPartsN *partsN(pick_nan)(FloatPartsN *a, FloatPartsN *b,
          * return the NaN with the positive sign bit (if any).
          */
         if (is_snan(a->cls)) {
-            if (is_snan(b->cls)) {
-                which = cmp > 0 ? 0 : 1;
-            } else {
+            if (!is_snan(b->cls)) {
                 which = is_qnan(b->cls) ? 1 : 0;
+                break;
             }
         } else if (is_qnan(a->cls)) {
             if (is_snan(b->cls) || !is_qnan(b->cls)) {
                 which = 0;
-            } else {
-                which = cmp > 0 ? 0 : 1;
+                break;
             }
         } else {
             which = 1;
+            break;
         }
+        cmp = frac_cmp(a, b);
+        if (cmp == 0) {
+            cmp = a->sign < b->sign;
+        }
+        which = cmp > 0 ? 0 : 1;
         break;
     default:
         g_assert_not_reached();