[middle-end,patch] Making __builtin_signbit type-generic

[middle-end,patch] Making __builtin_signbit type-generic

[FX]

[-- Attachment #1: Type: text/plain, Size: 369 bytes --]

Attached patch makes __builtin_signbit type-generic in the middle-end (PR 36757).
Error message will be issued (from gcc/c-family/c-common.c) if argument is not real (or too few, or too many).
gcc_assert() is used in expand_builtin_signbit() it cases that should be unreachable (failure to expand inline).

Tested on x86_64-apple-darwin14, OK to commit?

FX



[-- Attachment #2: signbit.ChangeLog.txt --]
[-- Type: text/plain, Size: 724 bytes --]

gcc/

2015-08-17  Francois-Xavier Coudert  <fxcoudert@gcc.gnu.org>

	PR middle-end/36757
	* builtins.c (expand_builtin_signbit): Add asserts to make sure
	we can expand BUILT_IN_SIGNBIT inline.
	* builtins.def (BUILT_IN_SIGNBIT): Make type-generic.
	* doc/extend.texi: Document the type-generic __builtin_signbit.

gcc/c-family/

2015-08-17  Francois-Xavier Coudert  <fxcoudert@gcc.gnu.org>

	PR middle-end/36757
	* c-common.c (check_builtin_function_arguments): Add check
	for BUILT_IN_SIGNBIT argument.

gcc/testsuite/

2015-08-17  Francois-Xavier Coudert  <fxcoudert@gcc.gnu.org>

	PR middle-end/36757
	* gcc.dg/builtins-error.c: Add checks for __builtin_signbit.
	* gcc.dg/tg-tests.h: Add checks for __builtin_signbit.


[-- Attachment #3: signbit.diff --]
[-- Type: application/octet-stream, Size: 7865 bytes --]

Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c	(revision 226894)
+++ gcc/builtins.c	(working copy)
@@ -4953,11 +4953,9 @@ expand_builtin_adjust_trampoline (tree e
    function.  The function first checks whether the back end provides
    an insn to implement signbit for the respective mode.  If not, it
    checks whether the floating point format of the value is such that
-   the sign bit can be extracted.  If that is not the case, the
-   function returns NULL_RTX to indicate that a normal call should be
-   emitted rather than expanding the function in-line.  EXP is the
-   expression that is a call to the builtin function; if convenient,
-   the result should be placed in TARGET.  */
+   the sign bit can be extracted.  If that is not the case, error out.
+   EXP is the expression that is a call to the builtin function; if
+   convenient, the result should be placed in TARGET.  */
 static rtx
 expand_builtin_signbit (tree exp, rtx target)
 {
@@ -5000,8 +4998,7 @@ expand_builtin_signbit (tree exp, rtx ta
   if (bitpos < 0)
   {
     /* But we can't do this if the format supports signed zero.  */
-    if (fmt->has_signed_zero && HONOR_SIGNED_ZEROS (fmode))
-      return NULL_RTX;
+    gcc_assert (!fmt->has_signed_zero || !HONOR_SIGNED_ZEROS (fmode));
 
     arg = fold_build2_loc (loc, LT_EXPR, TREE_TYPE (exp), arg,
 		       build_real (TREE_TYPE (arg), dconst0));
@@ -5011,8 +5008,7 @@ expand_builtin_signbit (tree exp, rtx ta
   if (GET_MODE_SIZE (fmode) <= UNITS_PER_WORD)
     {
       imode = int_mode_for_mode (fmode);
-      if (imode == BLKmode)
-	return NULL_RTX;
+      gcc_assert (imode != BLKmode);
       temp = gen_lowpart (imode, temp);
     }
   else
Index: gcc/builtins.def
===================================================================
--- gcc/builtins.def	(revision 226894)
+++ gcc/builtins.def	(working copy)
@@ -489,7 +489,7 @@ DEF_C99_BUILTIN        (BUILT_IN_SCALBLN
 DEF_C99_BUILTIN        (BUILT_IN_SCALBN, "scalbn", BT_FN_DOUBLE_DOUBLE_INT, ATTR_MATHFN_FPROUNDING_ERRNO)
 DEF_C99_BUILTIN        (BUILT_IN_SCALBNF, "scalbnf", BT_FN_FLOAT_FLOAT_INT, ATTR_MATHFN_FPROUNDING_ERRNO)
 DEF_C99_BUILTIN        (BUILT_IN_SCALBNL, "scalbnl", BT_FN_LONGDOUBLE_LONGDOUBLE_INT, ATTR_MATHFN_FPROUNDING_ERRNO)
-DEF_EXT_LIB_BUILTIN    (BUILT_IN_SIGNBIT, "signbit", BT_FN_INT_DOUBLE, ATTR_CONST_NOTHROW_LEAF_LIST)
+DEF_EXT_LIB_BUILTIN    (BUILT_IN_SIGNBIT, "signbit", BT_FN_INT_VAR, ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF)
 DEF_EXT_LIB_BUILTIN    (BUILT_IN_SIGNBITF, "signbitf", BT_FN_INT_FLOAT, ATTR_CONST_NOTHROW_LEAF_LIST)
 DEF_EXT_LIB_BUILTIN    (BUILT_IN_SIGNBITL, "signbitl", BT_FN_INT_LONGDOUBLE, ATTR_CONST_NOTHROW_LEAF_LIST)
 DEF_EXT_LIB_BUILTIN    (BUILT_IN_SIGNBITD32, "signbitd32", BT_FN_INT_DFLOAT32, ATTR_CONST_NOTHROW_LEAF_LIST)
Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 226894)
+++ gcc/c-family/c-common.c	(working copy)
@@ -10151,6 +10151,7 @@ check_builtin_function_arguments (tree f
     case BUILT_IN_ISINF_SIGN:
     case BUILT_IN_ISNAN:
     case BUILT_IN_ISNORMAL:
+    case BUILT_IN_SIGNBIT:
       if (builtin_function_validate_nargs (fndecl, nargs, 1))
 	{
 	  if (TREE_CODE (TREE_TYPE (args[0])) != REAL_TYPE)
Index: gcc/doc/extend.texi
===================================================================
--- gcc/doc/extend.texi	(revision 226894)
+++ gcc/doc/extend.texi	(working copy)
@@ -10448,7 +10448,7 @@ the same names as the standard macros ( 
 prefixed.  We intend for a library implementor to be able to simply
 @code{#define} each standard macro to its built-in equivalent.
 In the same fashion, GCC provides @code{fpclassify}, @code{isfinite},
-@code{isinf_sign} and @code{isnormal} built-ins used with
+@code{isinf_sign}, @code{isnormal} and @code{signbit} built-ins used with
 @code{__builtin_} prefixed.  The @code{isinf} and @code{isnan}
 built-in functions appear both with and without the @code{__builtin_} prefix.
 
Index: gcc/testsuite/gcc.dg/builtins-error.c
===================================================================
--- gcc/testsuite/gcc.dg/builtins-error.c	(revision 226894)
+++ gcc/testsuite/gcc.dg/builtins-error.c	(working copy)
@@ -16,6 +16,7 @@ int test1(struct X x)
   if (x.x == 10) return __builtin_islessequal(x, x); /* { dg-error "non-floating-point arguments" } */
   if (x.x == 11) return __builtin_islessgreater(x, x); /* { dg-error "non-floating-point arguments" } */
   if (x.x == 12) return __builtin_isunordered(x, x); /* { dg-error "non-floating-point arguments" } */
+  if (x.x == 13) return __builtin_signbit(x); /* { dg-error "non-floating-point argument" } */
 
   return 0;
 }
@@ -34,6 +35,7 @@ int test2(double x)
   if (x == 10) return __builtin_islessequal(x); /* { dg-error "not enough arguments" } */
   if (x == 11) return __builtin_islessgreater(x); /* { dg-error "not enough arguments" } */
   if (x == 12) return __builtin_isunordered(x); /* { dg-error "not enough arguments" } */
+  if (x == 13) return __builtin_signbit(); /* { dg-error "not enough arguments" } */
   return 0;
 }
 
@@ -51,6 +53,7 @@ int test3(double x)
   if (x == 10) return __builtin_islessequal(x, x, x); /* { dg-error "too many arguments" } */
   if (x == 11) return __builtin_islessgreater(x, x, x); /* { dg-error "too many arguments" } */
   if (x == 12) return __builtin_isunordered(x, x, x); /* { dg-error "too many arguments" } */
+  if (x == 13) return __builtin_signbit(x, x); /* { dg-error "too many arguments" } */
   return 0;
 }
 
Index: gcc/testsuite/gcc.dg/tg-tests.h
===================================================================
--- gcc/testsuite/gcc.dg/tg-tests.h	(revision 226894)
+++ gcc/testsuite/gcc.dg/tg-tests.h	(working copy)
@@ -11,7 +11,7 @@ void __attribute__ ((__noinline__))
 foo_1 (float f, double d, long double ld,
        int res_unord, int res_isnan, int res_isinf,
        int res_isinf_sign, int res_isfin, int res_isnorm,
-       int classification)
+       int res_signbit, int classification)
 {
   if (__builtin_isunordered (f, 0) != res_unord)
     __builtin_abort ();
@@ -80,6 +80,24 @@ foo_1 (float f, double d, long double ld
   if (__builtin_finitel (ld) != res_isfin)
     __builtin_abort ();
 
+  /* Sign bit of zeros and nans is not preserved in unsafe math mode.  */
+#ifdef UNSAFE
+  if (!res_isnan && d != 0)
+#endif
+    {
+      __builtin_printf ("%d %d %g\n", __builtin_signbit (d), res_signbit, d);
+      if ((__builtin_signbit (f) ? 1 : 0) != res_signbit)
+	__builtin_abort ();
+      if ((__builtin_signbit (d) ? 1 : 0) != res_signbit)
+	__builtin_abort ();
+      if ((__builtin_signbit (ld) ? 1 : 0) != res_signbit)
+	__builtin_abort ();
+      if ((__builtin_signbitf (f) ? 1 : 0) != res_signbit)
+	__builtin_abort ();
+      if ((__builtin_signbitl (ld) ? 1 : 0) != res_signbit)
+	__builtin_abort ();
+    }
+
   /* Subnormals can abruptly underflow to zero in unsafe math
      mode, so bypass testing these numbers if necessary.  */
 #ifdef UNSAFE
@@ -100,9 +118,10 @@ foo (float f, double d, long double ld,
      int res_unord, int res_isnan, int res_isinf,
      int res_isfin, int res_isnorm, int classification)
 {
-  foo_1 (f, d, ld, res_unord, res_isnan, res_isinf, res_isinf, res_isfin, res_isnorm, classification);
-  /* Try all the values negated as well.  */
-  foo_1 (-f, -d, -ld, res_unord, res_isnan, res_isinf, -res_isinf, res_isfin, res_isnorm, classification);
+  foo_1 (f, d, ld, res_unord, res_isnan, res_isinf, res_isinf, res_isfin, res_isnorm, 0, classification);
+  /* Try all the values negated as well.  All will have the sign bit set,
+     except for the nan.  */
+  foo_1 (-f, -d, -ld, res_unord, res_isnan, res_isinf, -res_isinf, res_isfin, res_isnorm, 1, classification);
 }
 
 int __attribute__ ((__noinline__))

Re: [middle-end,patch] Making __builtin_signbit type-generic

[Jeff Law]

On 08/17/2015 02:14 PM, FX wrote:
> Attached patch makes __builtin_signbit type-generic in the middle-end (PR 36757).
> Error message will be issued (from gcc/c-family/c-common.c) if argument is not real (or too few, or too many).
> gcc_assert() is used in expand_builtin_signbit() it cases that should be unreachable (failure to expand inline).
>
> Tested on x86_64-apple-darwin14, OK to commit?
>
> FX
>
>
>
> signbit.ChangeLog.txt
>
>
> gcc/
>
> 2015-08-17  Francois-Xavier Coudert<fxcoudert@gcc.gnu.org>
>
> 	PR middle-end/36757
> 	* builtins.c (expand_builtin_signbit): Add asserts to make sure
> 	we can expand BUILT_IN_SIGNBIT inline.
> 	* builtins.def (BUILT_IN_SIGNBIT): Make type-generic.
> 	* doc/extend.texi: Document the type-generic __builtin_signbit.
>
> gcc/c-family/
>
> 2015-08-17  Francois-Xavier Coudert<fxcoudert@gcc.gnu.org>
>
> 	PR middle-end/36757
> 	* c-common.c (check_builtin_function_arguments): Add check
> 	for BUILT_IN_SIGNBIT argument.
>
> gcc/testsuite/
>
> 2015-08-17  Francois-Xavier Coudert<fxcoudert@gcc.gnu.org>
>
> 	PR middle-end/36757
> 	* gcc.dg/builtins-error.c: Add checks for __builtin_signbit.
> 	* gcc.dg/tg-tests.h: Add checks for __builtin_signbit.

>> Index: gcc/testsuite/gcc.dg/tg-tests.h
> ===================================================================
> --- gcc/testsuite/gcc.dg/tg-tests.h	(revision 226894)
> +++ gcc/testsuite/gcc.dg/tg-tests.h	(working copy)
> @@ -11,7 +11,7 @@ void __attribute__ ((__noinline__))
>   foo_1 (float f, double d, long double ld,
>          int res_unord, int res_isnan, int res_isinf,
>          int res_isinf_sign, int res_isfin, int res_isnorm,
> -       int classification)
> +       int res_signbit, int classification)
>   {
>     if (__builtin_isunordered (f, 0) != res_unord)
>       __builtin_abort ();
> @@ -80,6 +80,24 @@ foo_1 (float f, double d, long double ld
>     if (__builtin_finitel (ld) != res_isfin)
>       __builtin_abort ();
>
> +  /* Sign bit of zeros and nans is not preserved in unsafe math mode.  */
> +#ifdef UNSAFE
> +  if (!res_isnan && d != 0)
> +#endif
> +    {
> +      __builtin_printf ("%d %d %g\n", __builtin_signbit (d), res_signbit, d);
Did you mean to keep this call to __builtin_printf?

OK with that removed, assuming it wasn't intentional.

jeff

Re: [middle-end,patch] Making __builtin_signbit type-generic

[FX]

>> +      __builtin_printf ("%d %d %g\n", __builtin_signbit (d), res_signbit, d);
> Did you mean to keep this call to __builtin_printf?
> OK with that removed, assuming it wasn't intentional.

Indeed, that was a leftover from a debugging session.

Committed to trunk, with that __builtin_printf removed, as revision 226990.
Thanks for the review.

FX

Re: [middle-end,patch] Making __builtin_signbit type-generic

[Paolo Carlini]

.. I think this improvement means that in principle we could revert 
what we committed for libstdc++/58625, thus increasing a little the 
consistency wrt the other classification facilities in c_global/cmath 
(and c_std/cmath). Not sure it's worth it.

Jon?

Thanks,
Paolo.

Re: [middle-end,patch] Making __builtin_signbit type-generic

[FX]

> .. I think this improvement means that in principle we could revert what we committed for libstdc++/58625, thus increasing a little the consistency wrt the other classification facilities in c_global/cmath (and c_std/cmath). Not sure it's worth it.

Can’t comment on whether it’s worth doing, but yes, with the type-generic __builtin_signbit you can revert that patch with no change to the generated code.

FX

Re: [middle-end,patch] Making __builtin_signbit type-generic

[Paolo Carlini]

Hi,

On 08/19/2015 08:50 AM, FX wrote:
>> .. I think this improvement means that in principle we could revert what we committed for libstdc++/58625, thus increasing a little the consistency wrt the other classification facilities in c_global/cmath (and c_std/cmath). Not sure it's worth it.
> CanÂ’t comment on whether itÂ’s worth doing, but yes, with the type-generic __builtin_signbit you can revert that patch with no change to the generated code.
Let's do it then! I'll post the trivial patchlet later today before 
committing.

Paolo.

Re: [middle-end,patch] Making __builtin_signbit type-generic

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 86 bytes --]

... I'm committing the below. Tested x86_64-linux.

Thanks,
Paolo.

/////////////////

[-- Attachment #2: CL --]
[-- Type: text/plain, Size: 176 bytes --]

2015-08-19  Paolo Carlini  <paolo.carlini@oracle.com>

	* include/c_global/cmath: Revert fix for libstdc++/58625, no
	longer necessary (__builtin_signbit is now type-generic).

[-- Attachment #3: patchlet --]
[-- Type: text/plain, Size: 826 bytes --]

Index: include/c_global/cmath
===================================================================
--- include/c_global/cmath	(revision 227003)
+++ include/c_global/cmath	(working copy)
@@ -650,10 +650,10 @@
     isnormal(_Tp __x)
     { return __x != 0 ? true : false; }
 
-  // The front-end doesn't provide a type generic builtin (libstdc++/58625).
+  // Note: c++/36757 is fixed, __builtin_signbit is type-generic.
   constexpr bool
   signbit(float __x)
-  { return __builtin_signbitf(__x); }
+  { return __builtin_signbit(__x); }
 
   constexpr bool
   signbit(double __x)
@@ -661,7 +661,7 @@
 
   constexpr bool
   signbit(long double __x)
-  { return __builtin_signbitl(__x); }
+  { return __builtin_signbit(__x); }
 
   template<typename _Tp>
     constexpr typename __gnu_cxx::__enable_if<__is_integer<_Tp>::__value,

Re: [middle-end,patch] Making __builtin_signbit type-generic

[Andreas Schwab]

FX <fxcoudert@gmail.com> writes:

> @@ -80,6 +80,24 @@ foo_1 (float f, double d, long double ld
>    if (__builtin_finitel (ld) != res_isfin)
>      __builtin_abort ();
>  
> +  /* Sign bit of zeros and nans is not preserved in unsafe math mode.  */
> +#ifdef UNSAFE
> +  if (!res_isnan && d != 0)
> +#endif

Why only in usafe mode?  Isn't the sign bit of NaN always unreliable?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [middle-end,patch] Making __builtin_signbit type-generic

[Joseph Myers]

On Wed, 19 Aug 2015, Andreas Schwab wrote:

> FX <fxcoudert@gmail.com> writes:
> 
> > @@ -80,6 +80,24 @@ foo_1 (float f, double d, long double ld
> >    if (__builtin_finitel (ld) != res_isfin)
> >      __builtin_abort ();
> >  
> > +  /* Sign bit of zeros and nans is not preserved in unsafe math mode.  */
> > +#ifdef UNSAFE
> > +  if (!res_isnan && d != 0)
> > +#endif
> 
> Why only in usafe mode?  Isn't the sign bit of NaN always unreliable?

NaN sign bits are meaningful for a limited set of operations.  (As I noted 
in <https://gcc.gnu.org/ml/gcc-patches/2010-11/msg01474.html>, there would 
be a use for a -fno-signed-nans option in certain cases, but we don't 
currently have such an option.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [middle-end,patch] Making __builtin_signbit type-generic

[Andreas Schwab]

Joseph Myers <joseph@codesourcery.com> writes:

> On Wed, 19 Aug 2015, Andreas Schwab wrote:
>
>> FX <fxcoudert@gmail.com> writes:
>> 
>> > @@ -80,6 +80,24 @@ foo_1 (float f, double d, long double ld
>> >    if (__builtin_finitel (ld) != res_isfin)
>> >      __builtin_abort ();
>> >  
>> > +  /* Sign bit of zeros and nans is not preserved in unsafe math mode.  */
>> > +#ifdef UNSAFE
>> > +  if (!res_isnan && d != 0)
>> > +#endif
>> 
>> Why only in usafe mode?  Isn't the sign bit of NaN always unreliable?
>
> NaN sign bits are meaningful for a limited set of operations.

And what are those?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [middle-end,patch] Making __builtin_signbit type-generic

[Joseph Myers]

On Wed, 19 Aug 2015, Andreas Schwab wrote:

> >> Why only in usafe mode?  Isn't the sign bit of NaN always unreliable?
> >
> > NaN sign bits are meaningful for a limited set of operations.
> 
> And what are those?

Assignment to the same type, negation, absolute value, copysign, signbit.  
(In particular, the sign bit of a NaN resulting from an arithmetic 
operation is unspecified, but it does have to act as if it has some 
particular sign; if the statement "a = b * c;" results in a containing a 
NaN, two calls to signbit (a), with a not modified in between, must result 
in the same sign being indicated, but what that value is doesn't matter, 
and signbit (-a) must result in the other sign being indicated.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [middle-end,patch] Making __builtin_signbit type-generic

[Andreas Schwab]

Joseph Myers <joseph@codesourcery.com> writes:

> On Wed, 19 Aug 2015, Andreas Schwab wrote:
>
>> >> Why only in usafe mode?  Isn't the sign bit of NaN always unreliable?
>> >
>> > NaN sign bits are meaningful for a limited set of operations.
>> 
>> And what are those?
>
> Assignment to the same type, negation, absolute value, copysign, signbit.  

Thanks, that means I have to fix the fpu emulation.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."