Patchwork softfloat: Handle float_muladd_negate_c when product is zero

login
register
mail settings
Submitter Richard Sandiford
Date Jan. 21, 2013, 9:32 p.m.
Message ID <87ehhekyru.fsf@talisman.default>
Download mbox | patch
Permalink /patch/214278/
State New
Headers show

Comments

Richard Sandiford - Jan. 21, 2013, 9:32 p.m.
Honour float_muladd_negate_c in the case where the product is zero and
c is nonzero.  Previously we would fail to negate c.

Seen in (and tested against) the gfortran testsuite on MIPS.

Signed-off-by: Richard Sandiford <rdsandiford@googlemail.com>
---
 fpu/softfloat.c | 6 ++++++
 1 file changed, 6 insertions(+)
Richard Sandiford - Jan. 21, 2013, 9:38 p.m.
BTW, I'm not sure it's right to be using *_muladd for MIPS.  MADD.fmt &
co. were fused operations in the early MIPS IV processors, but they've
had an intermediate rounding step since then (i.e. they're equivalent
to a separate multiplication and addition).  I'm not feeling brave
enough to tackle that though.

Richard
Peter Maydell - Jan. 21, 2013, 10:05 p.m.
On 21 January 2013 21:32, Richard Sandiford <rdsandiford@googlemail.com> wrote:
> Honour float_muladd_negate_c in the case where the product is zero and
> c is nonzero.  Previously we would fail to negate c.
>
> Seen in (and tested against) the gfortran testsuite on MIPS.
>
> Signed-off-by: Richard Sandiford <rdsandiford@googlemail.com>
> ---
>  fpu/softfloat.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index ac3d150..0028415 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -2234,6 +2234,9 @@ float32 float32_muladd(float32 a, float32 b, float32 c, int flags STATUS_PARAM)
>              }
>          }
>          /* Zero plus something non-zero : just return the something */
> +        if (flags & float_muladd_negate_c) {
> +            signflip ^= 1;
> +        }
>          return make_float32(float32_val(c) ^ (signflip << 31));

This is a correct change in that it fixes a definite bug and gives
the right results, but I wonder if it might be clearer to instead
change the return to read:

     return packFloat32(cSign ^ signflip, cExp, cSig);

?

That would mean we consistently handle the negate_c flag by:
 * flip cSign as soon as we split c into its component fields
 * never refer to c again

(the source of the bug here is me trying to be clever and avoid
reassembling the float, but forgetting that cSign might have
changed.)

-- PMM
Richard Sandiford - Jan. 21, 2013, 10:20 p.m.
Peter Maydell <peter.maydell@linaro.org> writes:
> On 21 January 2013 21:32, Richard Sandiford <rdsandiford@googlemail.com> wrote:
>> Honour float_muladd_negate_c in the case where the product is zero and
>> c is nonzero.  Previously we would fail to negate c.
>>
>> Seen in (and tested against) the gfortran testsuite on MIPS.
>>
>> Signed-off-by: Richard Sandiford <rdsandiford@googlemail.com>
>> ---
>>  fpu/softfloat.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
>> index ac3d150..0028415 100644
>> --- a/fpu/softfloat.c
>> +++ b/fpu/softfloat.c
>> @@ -2234,6 +2234,9 @@ float32 float32_muladd(float32 a, float32 b, float32 c, int flags STATUS_PARAM)
>>              }
>>          }
>>          /* Zero plus something non-zero : just return the something */
>> +        if (flags & float_muladd_negate_c) {
>> +            signflip ^= 1;
>> +        }
>>          return make_float32(float32_val(c) ^ (signflip << 31));
>
> This is a correct change in that it fixes a definite bug and gives
> the right results, but I wonder if it might be clearer to instead
> change the return to read:
>
>      return packFloat32(cSign ^ signflip, cExp, cSig);
>
> ?
>
> That would mean we consistently handle the negate_c flag by:
>  * flip cSign as soon as we split c into its component fields
>  * never refer to c again
>
> (the source of the bug here is me trying to be clever and avoid
> reassembling the float, but forgetting that cSign might have
> changed.)

Heh, wondered about that, but not knowing the code, I went for the
"safe" option.  I'll do it as you suggest, thanks.

Richard
Richard Sandiford - Jan. 22, 2013, 5:14 p.m.
Richard Sandiford <rdsandiford@googlemail.com> writes:
> BTW, I'm not sure it's right to be using *_muladd for MIPS.  MADD.fmt &
> co. were fused operations in the early MIPS IV processors, but they've
> had an intermediate rounding step since then (i.e. they're equivalent
> to a separate multiplication and addition).  I'm not feeling brave
> enough to tackle that though.

Actually, this turned out to be the cause of the remaining gfortran
testsuite failures.  Patch coming up...

Richard

Patch

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index ac3d150..0028415 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -2234,6 +2234,9 @@  float32 float32_muladd(float32 a, float32 b, float32 c, int flags STATUS_PARAM)
             }
         }
         /* Zero plus something non-zero : just return the something */
+        if (flags & float_muladd_negate_c) {
+            signflip ^= 1;
+        }
         return make_float32(float32_val(c) ^ (signflip << 31));
     }
 
@@ -3787,6 +3790,9 @@  float64 float64_muladd(float64 a, float64 b, float64 c, int flags STATUS_PARAM)
             }
         }
         /* Zero plus something non-zero : just return the something */
+        if (flags & float_muladd_negate_c) {
+            signflip ^= 1;
+        }
         return make_float64(float64_val(c) ^ ((uint64_t)signflip << 63));
     }