Patchwork Fix folding of logb (-Inf) (PR tree-optimization/57066)

login
register
mail settings
Submitter Marek Polacek
Date April 25, 2013, 1:23 p.m.
Message ID <20130425132350.GM13346@redhat.com>
Download mbox | patch
Permalink /patch/239509/
State New
Headers show

Comments

Marek Polacek - April 25, 2013, 1:23 p.m.
This is an attempt to fix PR57066, where when folding logb call,
we returned -Inf for logb(-Inf), which is not correct.

I had to adjust one testcase, because it checked for a wrong value.

What I don't know yet is what we should return for -Nan, it should be
in C9X standard, but I don't have it by hand.

Does this look sane?  So far regtested/bootstrapped on x86_64-linux.

2013-04-25  Marek Polacek  <polacek@redhat.com>

	* builtins.c (fold_builtin_logb): Return +Inf for -Inf.

	* gcc.dg/torture/builtin-logb-1.c: Adjust testcase.


	Marek
Marc Glisse - April 25, 2013, 1:37 p.m.
On Thu, 25 Apr 2013, Marek Polacek wrote:

> This is an attempt to fix PR57066, where when folding logb call,
> we returned -Inf for logb(-Inf), which is not correct.
>
> I had to adjust one testcase, because it checked for a wrong value.
>
> What I don't know yet is what we should return for -Nan, it should be
> in C9X standard, but I don't have it by hand.

Doesn't matter:

"Recommended practice

If a function with one or more NaN arguments returns a NaN result, the 
result should be the same as one of the NaN arguments (after possible type 
conversion), except perhaps for the sign."


"F.9.3.11 The logb functions
— logb(±0) returns −∞ and raises the ‘‘divide-by-zero’’ floating-point 
exception.
— logb(±∞) returns ∞."
Marek Polacek - April 25, 2013, 1:43 p.m.
On Thu, Apr 25, 2013 at 03:37:35PM +0200, Marc Glisse wrote:
> If a function with one or more NaN arguments returns a NaN result,
> the result should be the same as one of the NaN arguments (after
> possible type conversion), except perhaps for the sign."
> 
> 
> "F.9.3.11 The logb functions
> — logb(±0) returns −∞ and raises the ‘‘divide-by-zero’’
> floating-point exception.
> — logb(±∞) returns ∞."

Perfect, I'm glad to hear that.  Thanks!

	Marek
Richard Guenther - April 25, 2013, 1:44 p.m.
On Thu, Apr 25, 2013 at 3:23 PM, Marek Polacek <polacek@redhat.com> wrote:
> This is an attempt to fix PR57066, where when folding logb call,
> we returned -Inf for logb(-Inf), which is not correct.
>
> I had to adjust one testcase, because it checked for a wrong value.
>
> What I don't know yet is what we should return for -Nan, it should be
> in C9X standard, but I don't have it by hand.
>
> Does this look sane?  So far regtested/bootstrapped on x86_64-linux.
>
> 2013-04-25  Marek Polacek  <polacek@redhat.com>
>
>         * builtins.c (fold_builtin_logb): Return +Inf for -Inf.
>
>         * gcc.dg/torture/builtin-logb-1.c: Adjust testcase.
>
> --- gcc/builtins.c.mp   2013-04-25 12:52:37.463451032 +0200
> +++ gcc/builtins.c      2013-04-25 14:26:33.963099851 +0200
> @@ -9698,7 +9698,12 @@ fold_builtin_logb (location_t loc, tree
>        case rvc_inf:
>         /* If arg is Inf or NaN and we're logb, return it.  */
>         if (TREE_CODE (rettype) == REAL_TYPE)
> -         return fold_convert_loc (loc, rettype, arg);
> +         {
> +           /* For logb(-Inf) we have to return +Inf.  */
> +           if (value->cl == rvc_inf && !tree_expr_nonnegative_p (arg))
> +             real_inf (TREE_REAL_CST_PTR (arg));

Please don't modify arg in-place but simply use

             REAL_VALUE_TYPE tem;
             real_inf (&tem);
             return build_real (rettype, tem);

> +           return fold_convert_loc (loc, rettype, arg);
> +         }
>         /* Fall through... */
>        case rvc_zero:
>         /* Zero may set errno and/or raise an exception for logb, also
> --- gcc/testsuite/gcc.dg/torture/builtin-logb-1.c.mp    2013-04-25 13:23:18.408224450 +0200
> +++ gcc/testsuite/gcc.dg/torture/builtin-logb-1.c       2013-04-25 14:28:48.900534671 +0200
> @@ -48,19 +48,19 @@ extern void link_error(int);
>  /* Test if FUNCRES(FUNC(NEG FUNCARG(ARGARG))) is false.  Check the
>     sign as well.  */
>  #ifndef __SPU__
> -#define TESTIT3(FUNC,NEG,FUNCARG,ARGARG,FUNCRES) do { \
> +#define TESTIT3(FUNC,NEG,FUNCARG,ARGARG,FUNCRES,NEG2) do { \
>    if (!__builtin_##FUNCRES##f(__builtin_##FUNC(NEG __builtin_##FUNCARG##f(ARGARG))) \
> -      || CKSGN_F(__builtin_##FUNC##f(NEG __builtin_##FUNCARG##f(ARGARG)), NEG __builtin_##FUNCARG##f(ARGARG))) \
> +      || CKSGN_F(__builtin_##FUNC##f(NEG __builtin_##FUNCARG##f(ARGARG)), NEG2 __builtin_##FUNCARG##f(ARGARG))) \
>      link_error(__LINE__); \
>    if (!__builtin_##FUNCRES(__builtin_##FUNC(NEG __builtin_##FUNCARG(ARGARG))) \
> -      || CKSGN(__builtin_##FUNC(NEG __builtin_##FUNCARG(ARGARG)), NEG __builtin_##FUNCARG(ARGARG))) \
> +      || CKSGN(__builtin_##FUNC(NEG __builtin_##FUNCARG(ARGARG)), NEG2 __builtin_##FUNCARG(ARGARG))) \
>      link_error(__LINE__); \
>    if (!__builtin_##FUNCRES##l(__builtin_##FUNC##l(NEG __builtin_##FUNCARG##l(ARGARG))) \
> -      || CKSGN_L(__builtin_##FUNC##l(NEG __builtin_##FUNCARG##l(ARGARG)), NEG __builtin_##FUNCARG##l(ARGARG))) \
> +      || CKSGN_L(__builtin_##FUNC##l(NEG __builtin_##FUNCARG##l(ARGARG)), NEG2 __builtin_##FUNCARG##l(ARGARG))) \
>      link_error(__LINE__); \
>    } while (0)
>  #else
> -#define TESTIT3(FUNC,NEG,FUNCARG,ARGARG,FUNCRES) do { \
> +#define TESTIT3(FUNC,NEG,FUNCARG,ARGARG,FUNCRES,NEG2) do { \
>    /* SPU single-precision floating point format does not support Inf or Nan.  */ \
>    if (!__builtin_##FUNCRES(__builtin_##FUNC(NEG __builtin_##FUNCARG(ARGARG))) \
>        || CKSGN(__builtin_##FUNC(NEG __builtin_##FUNCARG(ARGARG)), NEG __builtin_##FUNCARG(ARGARG))) \
> @@ -173,15 +173,15 @@ foo(void)
>
>    /* Test for f(+-Inf) -> +-Inf and f(+-NaN) -> +-NaN, regardless of
>       the radix.  */
> -  TESTIT3 (logb, ,inf, , isinf);
> -  TESTIT3 (logb, - ,inf, , isinf);
> -  TESTIT3 (logb,  ,nan, "", isnan);
> -  TESTIT3 (logb, - ,nan, "", isnan);
> -
> -  TESTIT3 (significand, ,inf, , isinf);
> -  TESTIT3 (significand, - ,inf, , isinf);
> -  TESTIT3 (significand,  ,nan, "", isnan);
> -  TESTIT3 (significand, - ,nan, "", isnan);
> +  TESTIT3 (logb, ,inf, , isinf, );
> +  TESTIT3 (logb, - ,inf, , isinf, );
> +  TESTIT3 (logb,  ,nan, "", isnan, );
> +  TESTIT3 (logb, - ,nan, "", isnan, -);
> +
> +  TESTIT3 (significand, ,inf, , isinf, );
> +  TESTIT3 (significand, - ,inf, , isinf, -);
> +  TESTIT3 (significand,  ,nan, "", isnan, );
> +  TESTIT3 (significand, - ,nan, "", isnan, -);
>  }
>
>  int main()
>
>         Marek

Patch

--- gcc/builtins.c.mp	2013-04-25 12:52:37.463451032 +0200
+++ gcc/builtins.c	2013-04-25 14:26:33.963099851 +0200
@@ -9698,7 +9698,12 @@  fold_builtin_logb (location_t loc, tree
       case rvc_inf:
 	/* If arg is Inf or NaN and we're logb, return it.  */
 	if (TREE_CODE (rettype) == REAL_TYPE)
-	  return fold_convert_loc (loc, rettype, arg);
+	  {
+	    /* For logb(-Inf) we have to return +Inf.  */
+	    if (value->cl == rvc_inf && !tree_expr_nonnegative_p (arg))
+	      real_inf (TREE_REAL_CST_PTR (arg));
+	    return fold_convert_loc (loc, rettype, arg);
+	  }
 	/* Fall through... */
       case rvc_zero:
 	/* Zero may set errno and/or raise an exception for logb, also
--- gcc/testsuite/gcc.dg/torture/builtin-logb-1.c.mp	2013-04-25 13:23:18.408224450 +0200
+++ gcc/testsuite/gcc.dg/torture/builtin-logb-1.c	2013-04-25 14:28:48.900534671 +0200
@@ -48,19 +48,19 @@  extern void link_error(int);
 /* Test if FUNCRES(FUNC(NEG FUNCARG(ARGARG))) is false.  Check the
    sign as well.  */
 #ifndef __SPU__
-#define TESTIT3(FUNC,NEG,FUNCARG,ARGARG,FUNCRES) do { \
+#define TESTIT3(FUNC,NEG,FUNCARG,ARGARG,FUNCRES,NEG2) do { \
   if (!__builtin_##FUNCRES##f(__builtin_##FUNC(NEG __builtin_##FUNCARG##f(ARGARG))) \
-      || CKSGN_F(__builtin_##FUNC##f(NEG __builtin_##FUNCARG##f(ARGARG)), NEG __builtin_##FUNCARG##f(ARGARG))) \
+      || CKSGN_F(__builtin_##FUNC##f(NEG __builtin_##FUNCARG##f(ARGARG)), NEG2 __builtin_##FUNCARG##f(ARGARG))) \
     link_error(__LINE__); \
   if (!__builtin_##FUNCRES(__builtin_##FUNC(NEG __builtin_##FUNCARG(ARGARG))) \
-      || CKSGN(__builtin_##FUNC(NEG __builtin_##FUNCARG(ARGARG)), NEG __builtin_##FUNCARG(ARGARG))) \
+      || CKSGN(__builtin_##FUNC(NEG __builtin_##FUNCARG(ARGARG)), NEG2 __builtin_##FUNCARG(ARGARG))) \
     link_error(__LINE__); \
   if (!__builtin_##FUNCRES##l(__builtin_##FUNC##l(NEG __builtin_##FUNCARG##l(ARGARG))) \
-      || CKSGN_L(__builtin_##FUNC##l(NEG __builtin_##FUNCARG##l(ARGARG)), NEG __builtin_##FUNCARG##l(ARGARG))) \
+      || CKSGN_L(__builtin_##FUNC##l(NEG __builtin_##FUNCARG##l(ARGARG)), NEG2 __builtin_##FUNCARG##l(ARGARG))) \
     link_error(__LINE__); \
   } while (0)
 #else
-#define TESTIT3(FUNC,NEG,FUNCARG,ARGARG,FUNCRES) do { \
+#define TESTIT3(FUNC,NEG,FUNCARG,ARGARG,FUNCRES,NEG2) do { \
   /* SPU single-precision floating point format does not support Inf or Nan.  */ \
   if (!__builtin_##FUNCRES(__builtin_##FUNC(NEG __builtin_##FUNCARG(ARGARG))) \
       || CKSGN(__builtin_##FUNC(NEG __builtin_##FUNCARG(ARGARG)), NEG __builtin_##FUNCARG(ARGARG))) \
@@ -173,15 +173,15 @@  foo(void)
 
   /* Test for f(+-Inf) -> +-Inf and f(+-NaN) -> +-NaN, regardless of
      the radix.  */
-  TESTIT3 (logb, ,inf, , isinf);
-  TESTIT3 (logb, - ,inf, , isinf);
-  TESTIT3 (logb,  ,nan, "", isnan);
-  TESTIT3 (logb, - ,nan, "", isnan);
-
-  TESTIT3 (significand, ,inf, , isinf);
-  TESTIT3 (significand, - ,inf, , isinf);
-  TESTIT3 (significand,  ,nan, "", isnan);
-  TESTIT3 (significand, - ,nan, "", isnan);
+  TESTIT3 (logb, ,inf, , isinf, );
+  TESTIT3 (logb, - ,inf, , isinf, );
+  TESTIT3 (logb,  ,nan, "", isnan, );
+  TESTIT3 (logb, - ,nan, "", isnan, -);
+
+  TESTIT3 (significand, ,inf, , isinf, );
+  TESTIT3 (significand, - ,inf, , isinf, -);
+  TESTIT3 (significand,  ,nan, "", isnan, );
+  TESTIT3 (significand, - ,nan, "", isnan, -);
 }
 
 int main()