diff mbox

, Fix PR 71869, Correctly implement isgreater, etc. on PowerPC __float128

Message ID 20160726152338.GA7191@ibm-tiger.the-meissners.org
State New
Headers show

Commit Message

Michael Meissner July 26, 2016, 3:23 p.m. UTC 
When I originally developed the IEEE 128-bit floating point support, the
emulation routines in libgcc did not raise errors on signalling NaNs.  In the
course of adding full support for IEEE 128-bit floating point, we now have
added exception signaling support in the library.  This means the C99/IEEE
built-in functions isgreater, isgreaterequal, isless, islessequal, and
islessgreater now will raise an error when you compare a signaling NaN.  These
functions are mandated not to raise an error.

These patches add calls to __unordkf3 to validate that both arguments are
ordered before calling the ge/le/eq comparison function.  I have done
bootstraps and make check on both little endian Power8 and big endian Power7
(both 32-bit and 64-bit tests done on the Power7 box) with no regressions.  Are
these patches ok for the trunk?

In addition, since the glibc group needs this functionality to complete the
__float128 library support, I would like to get it into GCC 6.2 if it is still
possible.  I know this is last minute, but this patch is important to the GLIBC
group, and it was only noticed recently.  I will be starting the bootstrap and
regression test on the gcc6 branch shortly.  If the patch works on gcc6, can I
commit it to the gcc6 branch ASAP, or will it have to wait for GCC 6.3?

[gcc]
2016-07-25  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/71869
	* config/rs6000/rs6000.c (rs6000_generate_compare): Rework
	__float128 support when we don't have hardware support, so that
	the IEEE built-in functions like isgreater, first call __unordkf3
	to make sure neither operand is a NaN, and if both operands are
	ordered, do the normal comparison.

[gcc/testsuite]
2016-07-25  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/71869
	* gcc.target/powerpc/float128-cmp.c: New test to make sure that
	IEEE built-in functions first check to see if the arguments are
	ordered.

Comments

Segher Boessenkool July 26, 2016, 4:05 p.m. UTC | #1 
On Tue, Jul 26, 2016 at 11:23:38AM -0400, Michael Meissner wrote:
> When I originally developed the IEEE 128-bit floating point support, the
> emulation routines in libgcc did not raise errors on signalling NaNs.  In the
> course of adding full support for IEEE 128-bit floating point, we now have
> added exception signaling support in the library.  This means the C99/IEEE
> built-in functions isgreater, isgreaterequal, isless, islessequal, and
> islessgreater now will raise an error when you compare a signaling NaN.  These
> functions are mandated not to raise an error.
> 
> These patches add calls to __unordkf3 to validate that both arguments are
> ordered before calling the ge/le/eq comparison function.  I have done
> bootstraps and make check on both little endian Power8 and big endian Power7
> (both 32-bit and 64-bit tests done on the Power7 box) with no regressions.  Are
> these patches ok for the trunk?

They are okay for trunk (some testcase stuff below).

For the 6 branch, I'd rather see a little bit of testing on trunk first.
We still have some time (I hope).


> --- gcc/testsuite/gcc.target/powerpc/float128-cmp.c	(revision 0)
> +++ gcc/testsuite/gcc.target/powerpc/float128-cmp.c	(revision 0)
> @@ -0,0 +1,17 @@
> +/* { dg-do compile { target { powerpc*-*-linux* } } } */
> +/* { dg-require-effective-target powerpc_float128_sw_ok } */
> +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power7" } } */
> +/* { dg-options "-O2 -mcpu=power7 -mfloat128" } */
> +
> +#ifndef __FLOAT128__
> +#error "-mfloat128 is not supported."
> +#endif

This should never trigger, it is only there for debug, right?

> +int
> +test_isgreater (__float128 a, __float128 b)
> +{
> +  return __builtin_isgreater (a, b);
> +}
> +
> +/* { dg-final { scan-assembler "bl __\[gl\]ekf2"    } } */
> +/* { dg-final { scan-assembler "bl __unordkf2" } } */

There are some extra spaces in that [gl]ekf2 line.

Could you test all five functions please?  Use multiple testcases, maybe.

Thanks,


Segher
diff mbox

Patch

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 238730)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -21755,8 +21755,8 @@  rs6000_generate_compare (rtx cmp, machin
   else if (!TARGET_FLOAT128_HW && FLOAT128_VECTOR_P (mode))
     {
       rtx libfunc = NULL_RTX;
-      bool uneq_or_ltgt = false;
-      rtx dest = gen_reg_rtx (SImode);
+      bool check_nan = false;
+      rtx dest;
 
       switch (code)
 	{
@@ -21783,21 +21783,23 @@  rs6000_generate_compare (rtx cmp, machin
 
 	case UNGE:
 	case UNGT:
-	  libfunc = optab_libfunc (le_optab, mode);
+	  check_nan = true;
+	  libfunc = optab_libfunc (ge_optab, mode);
 	  code = (code == UNGE) ? GE : GT;
 	  break;
 
 	case UNLE:
 	case UNLT:
-	  libfunc = optab_libfunc (ge_optab, mode);
+	  check_nan = true;
+	  libfunc = optab_libfunc (le_optab, mode);
 	  code = (code == UNLE) ? LE : LT;
 	  break;
 
 	case UNEQ:
 	case LTGT:
-	  libfunc = optab_libfunc (le_optab, mode);
-	  uneq_or_ltgt = true;
-	  code = (code = UNEQ) ? NE : EQ;
+	  check_nan = true;
+	  libfunc = optab_libfunc (eq_optab, mode);
+	  code = (code = UNEQ) ? EQ : NE;
 	  break;
 
 	default:
@@ -21805,21 +21807,56 @@  rs6000_generate_compare (rtx cmp, machin
 	}
 
       gcc_assert (libfunc);
-      dest = emit_library_call_value (libfunc, NULL_RTX, LCT_CONST,
-				      SImode, 2, op0, mode, op1, mode);
 
-      /* If this is UNEQ or LTGT, we call __lekf2, which returns -1 for less
-	 than, 0 for equal, +1 for greater, and +2 for nan.  We add 1, to give
-	 a value of 0..3, and then do and AND immediate of 1 to isolate whether
-	 it is 0/Nan (i.e. bottom bit is 0), or less than/greater than
-	 (i.e. bottom bit is 1).  */
-      if (uneq_or_ltgt)
-	{
-	  rtx add_result = gen_reg_rtx (SImode);
-	  rtx and_result = gen_reg_rtx (SImode);
-	  emit_insn (gen_addsi3 (add_result, dest, GEN_INT (1)));
-	  emit_insn (gen_andsi3 (and_result, add_result, GEN_INT (1)));
-	  dest = and_result;
+      if (!check_nan)
+	dest = emit_library_call_value (libfunc, NULL_RTX, LCT_CONST,
+					SImode, 2, op0, mode, op1, mode);
+
+      /* The library signals an exception for signalling NaNs, so we need to
+	 handle isgreater, etc. by first checking isordered.  */
+      else
+	{
+	  rtx ne_rtx, normal_dest, unord_dest;
+	  rtx unord_func = optab_libfunc (unord_optab, mode);
+	  rtx join_label = gen_label_rtx ();
+	  rtx join_ref = gen_rtx_LABEL_REF (VOIDmode, join_label);
+	  rtx unord_cmp = gen_reg_rtx (comp_mode);
+
+
+	  /* Test for either value being a NaN.  */
+	  gcc_assert (unord_func);
+	  unord_dest = emit_library_call_value (unord_func, NULL_RTX, LCT_CONST,
+						SImode, 2, op0, mode, op1,
+						mode);
+
+	  /* Set value (0) if either value is a NaN, and jump to the join
+	     label.  */
+	  dest = gen_reg_rtx (SImode);
+	  emit_move_insn (dest, const1_rtx);
+	  emit_insn (gen_rtx_SET (unord_cmp,
+				  gen_rtx_COMPARE (comp_mode, unord_dest,
+						   const0_rtx)));
+
+	  ne_rtx = gen_rtx_NE (comp_mode, unord_cmp, const0_rtx);
+	  emit_jump_insn (gen_rtx_SET (pc_rtx,
+				       gen_rtx_IF_THEN_ELSE (VOIDmode, ne_rtx,
+							     join_ref,
+							     pc_rtx)));
+
+	  /* Do the normal comparison, knowing that the values are not
+	     NaNs.  */
+	  normal_dest = emit_library_call_value (libfunc, NULL_RTX, LCT_CONST,
+						 SImode, 2, op0, mode, op1,
+						 mode);
+
+	  emit_insn (gen_cstoresi4 (dest,
+				    gen_rtx_fmt_ee (code, SImode, normal_dest,
+						    const0_rtx),
+				    normal_dest, const0_rtx));
+
+	  /* Join NaN and non-Nan paths.  Compare dest against 0.  */
+	  emit_label (join_label);
+	  code = NE;
 	}
 
       emit_insn (gen_rtx_SET (compare_result,
Index: gcc/testsuite/gcc.target/powerpc/float128-cmp.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/float128-cmp.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/float128-cmp.c	(revision 0)
@@ -0,0 +1,17 @@ 
+/* { dg-do compile { target { powerpc*-*-linux* } } } */
+/* { dg-require-effective-target powerpc_float128_sw_ok } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power7" } } */
+/* { dg-options "-O2 -mcpu=power7 -mfloat128" } */
+
+#ifndef __FLOAT128__
+#error "-mfloat128 is not supported."
+#endif
+
+int
+test_isgreater (__float128 a, __float128 b)
+{
+  return __builtin_isgreater (a, b);
+}
+
+/* { dg-final { scan-assembler "bl __\[gl\]ekf2"    } } */
+/* { dg-final { scan-assembler "bl __unordkf2" } } */