PATCH RFA: Fix simulator handling of floating point absolute value

PATCH RFA: Fix simulator handling of floating point absolute value

[Ian Lance Taylor]

The common simulator routine sim_fpu_abs is used by MIPS, MN10300,
SH64, and perhaps some CGEN generated simulators (it is called by
cgen-accfp.c).  On most, and perhaps all, hardware, a floating point
absolute value instruction simply clears the sign bit.  However,
sim_fpu_abs does not clear the sign bit when given a NaN.  For MIPS,
this causes the gcc test gcc.c-torture/execute/ieee/copysign1.c to
fail.

This patch changes sim_fpu_abs to always clear the sign bit of the
argument.  It does not otherwise change the behaviour.  This, plus
another patch I am about to sign, fixes the gcc copysign1 test for
MIPS.

OK for mainline?

Ian


2005-07-07  Ian Lance Taylor  <ian@airs.com>

	* sim-fpu.c (sim_fpu_abs): Always clear the sign bit.


Index: common/sim-fpu.c
===================================================================
RCS file: /cvs/src/src/sim/common/sim-fpu.c,v
retrieving revision 1.8
diff -p -u -r1.8 sim-fpu.c
--- common/sim-fpu.c	22 Jun 2003 13:36:26 -0000	1.8
+++ common/sim-fpu.c	8 Jul 2005 05:05:12 -0000
@@ -1733,19 +1744,13 @@ INLINE_SIM_FPU (int)
 sim_fpu_abs (sim_fpu *f,
 	     const sim_fpu *r)
 {
+  *f = *r;
+  f->sign = 0;
   if (sim_fpu_is_snan (r))
     {
-      *f = *r;
       f->class = sim_fpu_class_qnan;
       return sim_fpu_status_invalid_snan;
     }
-  if (sim_fpu_is_qnan (r))
-    {
-      *f = *r;
-      return 0;
-    }
-  *f = *r;
-  f->sign = 0;
   return 0;
 }
 

Re: PATCH RFA: Fix simulator handling of floating point absolute value

[Daniel Jacobowitz]

On Thu, Jul 07, 2005 at 10:18:53PM -0700, Ian Lance Taylor wrote:
> The common simulator routine sim_fpu_abs is used by MIPS, MN10300,
> SH64, and perhaps some CGEN generated simulators (it is called by
> cgen-accfp.c).  On most, and perhaps all, hardware, a floating point
> absolute value instruction simply clears the sign bit.  However,
> sim_fpu_abs does not clear the sign bit when given a NaN.  For MIPS,
> this causes the gcc test gcc.c-torture/execute/ieee/copysign1.c to
> fail.
> 
> This patch changes sim_fpu_abs to always clear the sign bit of the
> argument.  It does not otherwise change the behaviour.  This, plus
> another patch I am about to sign, fixes the gcc copysign1 test for
> MIPS.
> 
> OK for mainline?

I have no references for this concern, but could you check that this
change is appropriate for at least MN10300 and SH64, since they share
ths code?


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: PATCH RFA: Fix simulator handling of floating point absolute value

[Frank Ch. Eigler]

ian wrote:

> [...]
> This patch changes sim_fpu_abs to always clear the sign bit of the
> argument.  It does not otherwise change the behaviour.  This, plus
> another patch I am about to sign, fixes the gcc copysign1 test for
> MIPS.
> 
> OK for mainline?
> [...]

Looks okay.  I can't find libgcc1 (soft-float emulation, the sim-fpu
code's ancestor) in current gcc any more to cross-check.  Newlib's
fabs implementation (libm/math/s_fabs.c) appears to clear the sign bit
in the same way as your new code.


- FChE

Re: PATCH RFA: Fix simulator handling of floating point absolute value

[Ian Lance Taylor]

Daniel Jacobowitz <drow@false.org> writes:

> On Thu, Jul 07, 2005 at 10:18:53PM -0700, Ian Lance Taylor wrote:
> > The common simulator routine sim_fpu_abs is used by MIPS, MN10300,
> > SH64, and perhaps some CGEN generated simulators (it is called by
> > cgen-accfp.c).  On most, and perhaps all, hardware, a floating point
> > absolute value instruction simply clears the sign bit.  However,
> > sim_fpu_abs does not clear the sign bit when given a NaN.  For MIPS,
> > this causes the gcc test gcc.c-torture/execute/ieee/copysign1.c to
> > fail.
> > 
> > This patch changes sim_fpu_abs to always clear the sign bit of the
> > argument.  It does not otherwise change the behaviour.  This, plus
> > another patch I am about to sign, fixes the gcc copysign1 test for
> > MIPS.
> > 
> > OK for mainline?
> 
> I have no references for this concern, but could you check that this
> change is appropriate for at least MN10300 and SH64, since they share
> ths code?

I think I'll need a little help to fully satisfy this request.  The
issue is how the fabs instruction on the MN10300 and SH handle NaNs.
This is something that can only be determined via a detailed processor
manual or by testing on real hardware.

The manual for the SH4a is, fortunately, clear: the fabs instruction
is described as "FRn & H'7FFF FFFF -> FRn."  So for the SH4a, at
least, my proposed patch is correct.

I found a processor manual for the MN10300 AM33 on the web, but all it
says about the fabs instruction is "This takes an absolute value of
the register (FSm), and stores the result in the register (FSn)."

A hardware designer implementing the floating point absolute
instruction has a choice.  He or she can implement a simple
instruction which clears the sign bit.  Or he or she can implement an
instruction which checks for NaN input, and, in that case, does not
clear the sign bit.  Since sim-fpu.c is intended to be for generic
use, I think it is reasonable to bet on the simple implementation, and
force a simulator for a processor which takes the complex
implementation to do something different.  Especially since it is
correct for at least two out of three processors.

Ian

Re: PATCH RFA: Fix simulator handling of floating point absolute value

[Daniel Jacobowitz]

On Fri, Jul 08, 2005 at 10:51:11AM -0700, Ian Lance Taylor wrote:
> > I have no references for this concern, but could you check that this
> > change is appropriate for at least MN10300 and SH64, since they share
> > ths code?
> 
> I think I'll need a little help to fully satisfy this request.  The
> issue is how the fabs instruction on the MN10300 and SH handle NaNs.
> This is something that can only be determined via a detailed processor
> manual or by testing on real hardware.
> 
> The manual for the SH4a is, fortunately, clear: the fabs instruction
> is described as "FRn & H'7FFF FFFF -> FRn."  So for the SH4a, at
> least, my proposed patch is correct.
> 
> I found a processor manual for the MN10300 AM33 on the web, but all it
> says about the fabs instruction is "This takes an absolute value of
> the register (FSm), and stores the result in the register (FSn)."
> 
> A hardware designer implementing the floating point absolute
> instruction has a choice.  He or she can implement a simple
> instruction which clears the sign bit.  Or he or she can implement an
> instruction which checks for NaN input, and, in that case, does not
> clear the sign bit.  Since sim-fpu.c is intended to be for generic
> use, I think it is reasonable to bet on the simple implementation, and
> force a simulator for a processor which takes the complex
> implementation to do something different.  Especially since it is
> correct for at least two out of three processors.

That's good enough for me; thank you, Ian.  Patch is OK.

-- 
Daniel Jacobowitz
CodeSourcery, LLC