diff mbox

FPU IEEE 754 for MIPS r5900

Message ID trinity-90c3148c-4066-484f-ae74-3ae10812591a-1374419835267@3capp-gmx-bs39
State New
Headers show

Commit Message

Jürgen Urban July 21, 2013, 3:17 p.m. UTC 
Hello Richard,

> "Jürgen Urban" <JuergenUrban@gmx.de> writes:
> >> "Jürgen Urban" <JuergenUrban@gmx.de> writes:
> >> >> "Jürgen Urban" <JuergenUrban@gmx.de> writes:
> >> >> > I used the SPU code in GCC as example for creating an
> >> >> > r5900_single_format structure. The patch is attached to the e-mail. I
> >> >> > want to submit this patch.
> >> >>
> >> >> Thanks.  Are there any real differences though?  E.g. in your version
> >> >> you set has_sign_dependent_rounding, but that's not necessary when the
> >> >> only rounding mode is towards zero.  has_sign_dependent_rounding says
> >> >> whether rounding X vs. -X can give numbers of different magnitude.
> >> >> (It was actually because of r5900 that this distinction was added.)
> >> >>
> >> >> I'm also not sure it makes sense to choose a different NaN encoding
> >> >> when NaNs aren't supported anyway.
> >> >>
> >> >> It would be good if we could reuse spu_single_format directly.
> >> >
> >> > I don't know what the effect of has_sign_dependent_rounding is.
> >>
> >> Like I say, it tells GCC whether -X can round to something other than -Y
> >> in cases where X would round to Y.  This is true for IEEE when rounding
> >> towards +infinity or -infinity, but those modes aren't supported on the
> >> R5900.
> >>
> >> Some transformations are invalid when has_sign_dependent is true.
> >> E.g. -(X - Y) is not always equal to Y - X.  We want it to be false
> >> when possible, so it looked like the spu_single_format version was right.
> >
> > The manual says that the rounding differs in the least significant bit,
> > so we need to assume that this can happen any time and it also leads to
> > a different result when the sign is different.
>
> Just to be clear, do you mean different from IEEE?  That doesn't matter
> for defining has_sign_dependent_rounding, which is comparing the R5900
> rounding of intermediate result -X vs. the R5900 rounding of intermediate
> result X.

I wasn't sure what sticky bits means. Now I got the information what the purpose of the sticky bits are. After reading the definition and testing, I think the rounding is not sign dependent. Ignoring sticky bits are only causing rounding towards 0.

> > Currently I have problems
> > testing it, because the latest GCC which I use (svn r200583) is not able
> > to build the Linux kernel, because it has at least 2 bugs. The first is
> > the bug 57698 introduced between 17.06. and 26.06. It seems that no GCC
> > developer is able to fix it. The second bug affects the dvb_demux.c from
> > Linux 2.6.34 when -Os and -mlong is used. This triggers an sanity check
> > in do_SUBST in file gcc/combine.c. The first bugs happens on all
> > architectures. The second also appears with normal mipsel. There is also
> > a bug which annoys me since years, __attribute__((aligned(16))) is not
> > working with local variables and doesn't print a warning. This is
> > particulary a problem when used in typedefs, because the problem is not
> > visible.
> > My native ps2sdk doesn't have an implementation of rounding. I don't
> > have an environment with a combination of the correct versions of the
> > different components for Linux. So I can't test it at the moment.
>
> OK, but the patch does need to be tested before it goes in.
> As far as 57698 goes, it would be fine to test with a 17.06 version.
>
> But why do you need to be able to build linux with gcc trunk to test this?
> The kernel doesn't use FP anyway.  Can't you just use testcases running
> under the kernel you already have?

The kernel disabled the FPU, because otherwise code from the official Fedora 12 release crashs. The FPU is emulated by the kernel. I needed to recompile it to enable the FPU. Now I used the old GCC to compile the kernel.

> >> > I also can't test it, because the GCC is already not correctly working
> >> > on SPU.
> >>
> >> Can you give an example?
> >
> > I wanted to say that I don't know what is correct or not, because the
> > GCC is already not handling the other stuff correctly, e.g.:
> > inf - inf => 0
> > nan - nan => 0
> > nan == nan => true (this must be false according to IEEE 754)
>
> Sorry, I was meaning in terms of source code.  I still think these
> expressions have no meaning for R5900 floats, where there isn't an
> infinity or a nan to begin with.  If the format doesn't have infinity
> to begin with, there's no right or wrong answer for "inf - inf"
> (__builtin_inff() - __builtin_inff()).  0x7f800000 is not inf,
> so what the real FPU does for 0x7f800000 doesn't affect things.
>
> When you construct 0x7f800000 as a finite value it seems to be
> handled correctly.  On spu-elf I tried:
>
> float f = 0x1.0p128f - 0x1.0p128f;
>
> and got zero as expected.  Note that the equivalent on x86_64 gives
> a warning:
>
> warning: floating constant exceeds range of ‘float’ [-Woverflow]
>
> and produces a NaN.  So it looks like this is working.
>
> > According to the documentation
> > http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html: __builting_inf()
> > should produce a warning if infinities are not supported. This doesn't
> > happen, so I need to assume that GCC thinks that it is supported and
> > emulates it, so it should work.
>
> I tried:
>
>   float f = __builtin_inff ();
>
> for spu-elf and got:
>
>   warning: target format does not support infinity [enabled by default]
>
> This doesn't give an error:
>
>   float f = __builtin_inf ();
>
> because doubles do have infinity.  Is that the problem you're seeing?

Yes.

> It's not obvious what should happen there though -- others would know
> better than me.  But it shouldn't occur in practice anyway.  The
> standard INFINITY macro always uses __builtin_inff, and the code was
> written for that assumption:
>
>   /* __builtin_inff is intended to be usable to define INFINITY on all
>      targets.  If an infinity is not available, INFINITY expands "to a
>      positive constant of type float that overflows at translation
>      time", footnote "In this case, using INFINITY will violate the
>      constraint in 6.4.4 and thus require a diagnostic." (C99 7.12#4).
>      Thus we pedwarn to ensure this constraint violation is
>      diagnosed.  */
>   if (!MODE_HAS_INFINITIES (TYPE_MODE (type)) && warn)
>     pedwarn (loc, 0, "target format does not support infinity");
>
> So using INFINITY should always give the warning.
>
> Directly using the double __builtin_inf instead of the float __builtin_inff
> to construct a float inf on a target that doesn't have float infs is a bit
> "doctor, it hurts if I do this".  I think we can ignore it for the
> initial patch.  Any existing code that converts __builtin_inf to float
> isn't going to expect it to be treated as a finite value anyway.
>
> I'm trying to make things simpler here, believe me :-)

I updated the patch to use the floating point format of the SPU. It seems that the format can be used and the FPU support is at least as good as on the SPU.

Best regards
Jürgen
diff mbox

Patch

Index: gcc/config.gcc
===================================================================
--- gcc/config.gcc	(Revision 200583)
+++ gcc/config.gcc	(Arbeitskopie)
@@ -3080,7 +3080,7 @@ 
   esac
 fi
 
-# Infer a default setting for --with-float.
+# Infer a default setting for --with-float and --with-fpu.
 if test x$with_float = x; then
   case ${target} in
     mips64r5900-*-* | mips64r5900el-*-* | mipsr5900-*-* | mipsr5900el-*-*)
@@ -3089,6 +3089,17 @@ 
       with_float=soft
       ;;
   esac
+else
+  case ${target} in
+    mips64r5900-*-* | mips64r5900el-*-* | mipsr5900-*-* | mipsr5900el-*-*)
+      if test $with_float = hard; then
+        if test x$with_fpu = x; then
+	  # The FPU of the R5900 is 32 bit.
+	  with_fpu=single
+        fi
+      fi
+      ;;
+  esac
 fi
 
 # Support --with-fpmath.
@@ -3469,7 +3480,7 @@ 
 		;;
 
 	mips*-*-*)
-		supported_defaults="abi arch arch_32 arch_64 float tune tune_32 tune_64 divide llsc mips-plt synci"
+		supported_defaults="abi arch arch_32 arch_64 float fpu tune tune_32 tune_64 divide llsc mips-plt synci"
 
 		case ${with_float} in
 		"" | soft | hard)
@@ -3481,6 +3492,16 @@ 
 			;;
 		esac
 
+		case ${with_fpu} in
+		"" | single | double)
+			# OK
+			;;
+		*)
+			echo "Unknown fpu type used in --with-fpu=$with_fpu" 1>&2
+			exit 1
+			;;
+		esac
+
 		case ${with_abi} in
 		"" | 32 | o64 | n32 | 64 | eabi)
 			# OK
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	(Revision 200583)
+++ gcc/config/mips/mips.c	(Arbeitskopie)
@@ -16830,6 +16830,19 @@ 
 	target_flags &= ~MASK_FLOAT64;
     }
 
+  if (TARGET_HARD_FLOAT_ABI && TARGET_FLOAT64 && TARGET_MIPS5900)
+    {
+      /* FPU of r5900 only supports 32 bit. */
+      error ("unsupported combination: %s", "-march=r5900 -mfp64 -mhard-float");
+    }
+
+  if (TARGET_HARD_FLOAT_ABI && TARGET_DOUBLE_FLOAT && TARGET_MIPS5900)
+    {
+      /* FPU of r5900 only supports 32 bit. */
+      error ("unsupported combination: %s",
+             "-march=r5900 -mdouble-float -mhard-float");
+    }
+
   /* End of code shared with GAS.  */
 
   /* If a -mlong* option was given, check that it matches the ABI,
@@ -17139,6 +17152,11 @@ 
      filling.  Registering the pass must be done at start up.  It's
      convenient to do it here.  */
   register_pass (&insert_pass_mips_machine_reorg2);
+
+  if (TARGET_HARD_FLOAT_ABI && TARGET_MIPS5900)
+    {
+      REAL_MODE_FORMAT (SFmode) = &spu_single_format;
+    }
 }
 
 /* Swap the register information for registers I and I + 1, which
Index: gcc/config/mips/mips.h
===================================================================
--- gcc/config/mips/mips.h	(Revision 200583)
+++ gcc/config/mips/mips.h	(Arbeitskopie)
@@ -754,6 +754,7 @@ 
   {"tune_64", "%{" OPT_ARCH64 ":%{!mtune=*:-mtune=%(VALUE)}}" }, \
   {"abi", "%{!mabi=*:-mabi=%(VALUE)}" }, \
   {"float", "%{!msoft-float:%{!mhard-float:-m%(VALUE)-float}}" }, \
+  {"fpu", "%{!msingle-float:%{!mdouble-float:-m%(VALUE)-float}}" }, \
   {"divide", "%{!mdivide-traps:%{!mdivide-breaks:-mdivide-%(VALUE)}}" }, \
   {"llsc", "%{!mllsc:%{!mno-llsc:-m%(VALUE)}}" }, \
   {"mips-plt", "%{!mplt:%{!mno-plt:-m%(VALUE)}}" }, \
@@ -859,7 +860,8 @@ 
 				 || TARGET_LOONGSON_2EF)
 
 /* ISA has LDC1 and SDC1.  */
-#define ISA_HAS_LDC1_SDC1	(!ISA_MIPS1 && !TARGET_MIPS16)
+#define ISA_HAS_LDC1_SDC1	(!ISA_MIPS1 && !TARGET_MIPS16           \
+				 && !TARGET_MIPS5900)
 
 /* ISA has the mips4 FP condition code instructions: FP-compare to CC,
    branch on CC, and move (both FP and non-FP) on CC.  */