[PATCH][PR 57371] Remove useless floating point casts in comparisons

[PATCH][PR 57371] Remove useless floating point casts in comparisons

[Yuri Gribov]

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

Hi all,

This is initial patch for
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57371 . Per Joseph's
suggestion it optimizes
  (float)lhs CMP rhs
  (double)lhs CMP rhs
to
  lhs CMP (typeof(x))rhs
whenever typeof(x) can be precisely represented by floating-point type
(e.g. short by float or int by double) and rhs can be precisely
represented by typeof(x).

Bootstrapped/regtested on x64. Ok for trunk?

I'd like to extend this further in follow-up patches:
1) fold always-false/always-true comparisons e.g.
  short x;
  (float)x > INT16_MAX;  // Always false
2) get rid of cast in comparisons with zero regardless of typeof(lhs)
when -fno-trapping-math:
  (float_or_double)lhs CMP 0

-Y

[-- Attachment #2: pr57371-1.patch --]
[-- Type: application/octet-stream, Size: 2858 bytes --]

2017-07-02  Yury Gribov  <tetra2005@gmail.com>

	PR tree-optimization/57371
	* match.pd: New pattern.
	* testsuite/gcc.dg/pr57371-1.c: New test.
	* testsuite/gcc.dg/pr57371-2.c: New test.

diff -rupN gcc/gcc/match.pd gcc-57371/gcc/match.pd
--- gcc/gcc/match.pd	2017-06-29 21:14:57.000000000 +0200
+++ gcc-57371/gcc/match.pd	2017-07-01 09:08:04.000000000 +0200
@@ -2802,7 +2802,35 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
    (simplify
     (cmp (sq @0) (sq @1))
       (if (! HONOR_NANS (@0))
-	(cmp @0 @1))))))
+	(cmp @0 @1)))))
+
+ /* Get rid of float cast in
+     (float_type)N CMP M
+    if N and M are within the range explicitly representable
+    by float type.
+
+    TODO: fold always true/false comparisons if M is outside valid range.  */
+ (simplify
+  (cmp (float @0) REAL_CST@1)
+  (if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (@1)))
+   (with
+    {
+      tree itype = TREE_TYPE (@0);
+
+      const real_format *fmt = REAL_MODE_FORMAT (TYPE_MODE (TREE_TYPE (@1)));
+
+      const REAL_VALUE_TYPE *rhs = TREE_REAL_CST_PTR (@1);
+      bool not_rhs_int_p = false;
+      wide_int rhs_int = real_to_integer (rhs, &not_rhs_int_p, TYPE_PRECISION (itype));
+    }
+    (if (!not_rhs_int_p
+         && !(TYPE_UNSIGNED (itype) && real_isneg (rhs))
+         && wi::ge_p (rhs_int, wi::min_value (itype), TYPE_SIGN (itype))
+         && wi::le_p (rhs_int, wi::max_value (itype), TYPE_SIGN (itype))
+         && TYPE_PRECISION (itype) <= significand_size (fmt))
+     (cmp @0 { wide_int_to_tree (itype, rhs_int); })
+    ))))
+)
 
 /* Fold A /[ex] B CMP C to A CMP B * C.  */
 (for cmp (eq ne)
diff -rupN gcc/gcc/testsuite/c-c++-common/pr57371-1.c gcc-57371/gcc/testsuite/c-c++-common/pr57371-1.c
--- gcc/gcc/testsuite/c-c++-common/pr57371-1.c	1970-01-01 01:00:00.000000000 +0100
+++ gcc-57371/gcc/testsuite/c-c++-common/pr57371-1.c	2017-07-01 10:00:36.000000000 +0200
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized" } */
+
+int foo1(short x) {
+  return (double)x != 0;
+}
+
+int foo2(short x) {
+  return (float)x != 0;
+}
+
+int foo3(int x) {
+  return (double)x != 0;
+}
+
+/* { dg-final { scan-tree-dump-not "\\(float\\)" "optimized" } } */
+/* { dg-final { scan-tree-dump-not "\\(double\\)" "optimized" } } */
diff -rupN gcc/gcc/testsuite/c-c++-common/pr57371-2.c gcc-57371/gcc/testsuite/c-c++-common/pr57371-2.c
--- gcc/gcc/testsuite/c-c++-common/pr57371-2.c	1970-01-01 01:00:00.000000000 +0100
+++ gcc-57371/gcc/testsuite/c-c++-common/pr57371-2.c	2017-07-01 10:00:32.000000000 +0200
@@ -0,0 +1,12 @@
+/* { dg-options "-O -fdump-tree-optimized" } */
+/* { dg-do compile } */
+
+int foo1(int x) {
+  /* { dg-final { scan-tree-dump "\\(float\\)" "optimized" } } */
+  return (float)x != 0;
+}
+
+int foo2(long long x) {
+  /* { dg-final { scan-tree-dump "\\(double\\)" "optimized" } } */
+  return (double)x != 0;
+}

Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons

[Marc Glisse]

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

On Sun, 2 Jul 2017, Yuri Gribov wrote:

> This is initial patch for
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57371 .

Thanks. I have had this unfinished, probably wrong patch on my hard drive 
for 4 years, I doubt there is much you can extract from it, but just in 
case...

-- 
Marc Glisse

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

Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c	(revision 209514)
+++ gcc/fold-const.c	(working copy)
@@ -6389,21 +6389,21 @@ fold_inf_compare (location_t loc, enum t
       /* x > +Inf is always false, if with ignore sNANs.  */
       if (HONOR_SNANS (mode))
         return NULL_TREE;
       return omit_one_operand_loc (loc, type, integer_zero_node, arg0);
 
     case LE_EXPR:
       /* x <= +Inf is always true, if we don't case about NaNs.  */
       if (! HONOR_NANS (mode))
 	return omit_one_operand_loc (loc, type, integer_one_node, arg0);
 
-      /* x <= +Inf is the same as x == x, i.e. isfinite(x).  */
+      /* x <= +Inf is the same as x == x, i.e. !isnan(x).  */
       arg0 = save_expr (arg0);
       return fold_build2_loc (loc, EQ_EXPR, type, arg0, arg0);
 
     case EQ_EXPR:
     case GE_EXPR:
       /* x == +Inf and x >= +Inf are always equal to x > DBL_MAX.  */
       real_maxval (&max, neg, mode);
       return fold_build2_loc (loc, neg ? LT_EXPR : GT_EXPR, type,
 			  arg0, build_real (TREE_TYPE (arg0), max));
 
@@ -9389,70 +9389,121 @@ fold_comparison (location_t loc, enum tr
 
   tem = maybe_canonicalize_comparison (loc, code, type, arg0, arg1);
   if (tem)
     return tem;
 
   if (FLOAT_TYPE_P (TREE_TYPE (arg0)))
     {
       tree targ0 = strip_float_extensions (arg0);
       tree targ1 = strip_float_extensions (arg1);
       tree newtype = TREE_TYPE (targ0);
+      tree ftype = TREE_TYPE (arg0);
 
       if (TYPE_PRECISION (TREE_TYPE (targ1)) > TYPE_PRECISION (newtype))
 	newtype = TREE_TYPE (targ1);
 
       /* Fold (double)float1 CMP (double)float2 into float1 CMP float2.  */
-      if (TYPE_PRECISION (newtype) < TYPE_PRECISION (TREE_TYPE (arg0)))
+      if (TYPE_PRECISION (newtype) < TYPE_PRECISION (ftype))
 	return fold_build2_loc (loc, code, type,
 			    fold_convert_loc (loc, newtype, targ0),
 			    fold_convert_loc (loc, newtype, targ1));
 
       /* (-a) CMP (-b) -> b CMP a  */
       if (TREE_CODE (arg0) == NEGATE_EXPR
 	  && TREE_CODE (arg1) == NEGATE_EXPR)
 	return fold_build2_loc (loc, code, type, TREE_OPERAND (arg1, 0),
 			    TREE_OPERAND (arg0, 0));
 
       if (TREE_CODE (arg1) == REAL_CST)
 	{
 	  REAL_VALUE_TYPE cst;
 	  cst = TREE_REAL_CST (arg1);
 
 	  /* (-a) CMP CST -> a swap(CMP) (-CST)  */
 	  if (TREE_CODE (arg0) == NEGATE_EXPR)
 	    return fold_build2_loc (loc, swap_tree_comparison (code), type,
 				TREE_OPERAND (arg0, 0),
-				build_real (TREE_TYPE (arg1),
+				build_real (ftype,
 					    real_value_negate (&cst)));
 
 	  /* IEEE doesn't distinguish +0 and -0 in comparisons.  */
 	  /* a CMP (-0) -> a CMP 0  */
 	  if (REAL_VALUE_MINUS_ZERO (cst))
 	    return fold_build2_loc (loc, code, type, arg0,
-				build_real (TREE_TYPE (arg1), dconst0));
+				build_real (ftype, dconst0));
 
 	  /* x != NaN is always true, other ops are always false.  */
 	  if (REAL_VALUE_ISNAN (cst)
-	      && ! HONOR_SNANS (TYPE_MODE (TREE_TYPE (arg1))))
+	      && ! HONOR_SNANS (TYPE_MODE (ftype)))
 	    {
 	      tem = (code == NE_EXPR) ? integer_one_node : integer_zero_node;
 	      return omit_one_operand_loc (loc, type, tem, arg0);
 	    }
 
 	  /* Fold comparisons against infinity.  */
 	  if (REAL_VALUE_ISINF (cst)
-	      && MODE_HAS_INFINITIES (TYPE_MODE (TREE_TYPE (arg1))))
+	      && MODE_HAS_INFINITIES (TYPE_MODE (ftype)))
 	    {
 	      tem = fold_inf_compare (loc, code, type, arg0, arg1);
 	      if (tem != NULL_TREE)
 		return tem;
 	    }
+
+	  /* (double)i CMP cst is often just i CMP cst'.
+	     See PR 57371 for a detailed analysis and other ideas.  */
+	  if (TREE_CODE (arg0) == FLOAT_EXPR)
+	    {
+	      tree inner = TREE_OPERAND (arg0, 0);
+	      tree itype = TREE_TYPE (inner);
+	      unsigned mantissa = significand_size (TYPE_MODE (ftype));
+	      unsigned prec = element_precision (itype);
+	      bool uns = TYPE_UNSIGNED (itype);
+	      bool fits = mantissa >= prec - !uns;
+	      /* If ftype cannot represent exactly all values of itype,
+		 we may have an inexact exception.  If the conversion from
+		 itype to ftype may overflow (unsigned __int128 to float),
+		 we may have an overflow exception.  */
+	      if (!flag_trapping_math || fits)
+		{
+		  /* Comparison with 0 is always fine.  */
+		  if (real_zerop (arg1))
+		    return fold_build2_loc (loc, code, type, inner,
+					    build_zero_cst (itype));
+
+		  /* (double)i == 2.5 is false.  This may assume that HUGE_VAL
+		     is an integer.  */
+		  if (!real_isinteger (&cst, TYPE_MODE (ftype)))
+		    {
+		      if (code == EQ_EXPR)
+			return omit_one_operand_loc (loc, type,
+						     integer_zero_node, arg0);
+		      if (code == NE_EXPR)
+			return omit_one_operand_loc (loc, type,
+						     integer_one_node, arg0);
+		    }
+
+		  /* floor(abs(cst))+1 is representable exactly in ftype.  */
+		  int log_cst = real_exponent (&cst);
+		  gcc_assert (REAL_MODE_FORMAT (TYPE_MODE (ftype))->b % 2 == 0);
+		  if (log_cst <= mantissa)
+		    {
+		      tree imin = TYPE_MIN_VALUE (itype);
+		      tree imax = TYPE_MAX_VALUE (itype);
+		      REAL_VALUE_TYPE iminf = real_value_from_int_cst (imin);
+		      REAL_VALUE_TYPE imaxf = real_value_from_int_cst (imax);
+		      if (REAL_VALUES_LESS (cst, iminf)
+			  || REAL_VALUES_LESS (imaxf, cst))
+			return build_int_cst (type,
+				 real_compare (code, &dconst0, &cst));
+		    }
+		}
+	    }
 	}
 
       /* If this is a comparison of a real constant with a PLUS_EXPR
 	 or a MINUS_EXPR of a real constant, we can convert it into a
 	 comparison with a revised real constant as long as no overflow
 	 occurs when unsafe_math_optimizations are enabled.  */
       if (flag_unsafe_math_optimizations
 	  && TREE_CODE (arg1) == REAL_CST
 	  && (TREE_CODE (arg0) == PLUS_EXPR
 	      || TREE_CODE (arg0) == MINUS_EXPR)
Index: gcc/testsuite/gcc.dg/pr57371-1.c
===================================================================
--- gcc/testsuite/gcc.dg/pr57371-1.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr57371-1.c	(working copy)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized -ftrapping-math" } */
+
+int f (char x)
+{
+  long double y = x;
+  return y <= 0;
+}
+
+/* { dg-final { scan-tree-dump-not "double" "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */

Property changes on: gcc/testsuite/gcc.dg/pr57371-1.c
___________________________________________________________________
Added: svn:eol-style
## -0,0 +1 ##
+native
\ No newline at end of property
Added: svn:keywords
## -0,0 +1 ##
+Author Date Id Revision URL
\ No newline at end of property
Index: gcc/testsuite/gcc.dg/pr57371-2.c
===================================================================
--- gcc/testsuite/gcc.dg/pr57371-2.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr57371-2.c	(working copy)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized -fno-trapping-math" } */
+
+int f (unsigned long long x)
+{
+  float y = x;
+  return y <= 0;
+}
+
+/* { dg-final { scan-tree-dump-not "float" "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */

Property changes on: gcc/testsuite/gcc.dg/pr57371-2.c
___________________________________________________________________
Added: svn:keywords
## -0,0 +1 ##
+Author Date Id Revision URL
\ No newline at end of property
Added: svn:eol-style
## -0,0 +1 ##
+native
\ No newline at end of property
Index: gcc/testsuite/gcc.dg/pr57371-3.c
===================================================================
--- gcc/testsuite/gcc.dg/pr57371-3.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr57371-3.c	(working copy)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O -fdump-tree-optimized -ftrapping-math" } */
+
+int f (unsigned long long x)
+{
+  float y = x;
+  return y <= 0;
+}
+
+/* { dg-final { scan-tree-dump "float" "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */

Property changes on: gcc/testsuite/gcc.dg/pr57371-3.c
___________________________________________________________________
Added: svn:keywords
## -0,0 +1 ##
+Author Date Id Revision URL
\ No newline at end of property
Added: svn:eol-style
## -0,0 +1 ##
+native
\ No newline at end of property

Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons

[Yuri Gribov]

On Sun, Jul 2, 2017 at 6:32 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Sun, 2 Jul 2017, Yuri Gribov wrote:
>
>> This is initial patch for
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57371 .
>
> Thanks. I have had this unfinished, probably wrong patch on my hard drive
> for 4 years, I doubt there is much you can extract from it, but just in
> case...

Thanks, this will indeed help! I only don't like !uns here:

+      bool uns = TYPE_UNSIGNED (itype);
+      bool fits = mantissa >= prec - !uns;

as I believe for INT?_MIN we need 'prec' number of bits.

-Y

Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons

[Marc Glisse]

On Sun, 2 Jul 2017, Yuri Gribov wrote:

> On Sun, Jul 2, 2017 at 6:32 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
>> On Sun, 2 Jul 2017, Yuri Gribov wrote:
>>
>>> This is initial patch for
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57371 .
>>
>> Thanks. I have had this unfinished, probably wrong patch on my hard drive
>> for 4 years, I doubt there is much you can extract from it, but just in
>> case...
>
> Thanks, this will indeed help! I only don't like !uns here:
>
> +      bool uns = TYPE_UNSIGNED (itype);
> +      bool fits = mantissa >= prec - !uns;
>
> as I believe for INT?_MIN we need 'prec' number of bits.

INT_MIN is (minus) a power of 2, so 1 bit of mantissa is sufficient to 
represent it exactly (as long as the exponent also fits). At least for an 
IEEE754 representation using base 2, but I expect non-IEEE representations 
of floats are similar enough.

-- 
Marc Glisse

Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons

[Yuri Gribov]

On Sun, Jul 2, 2017 at 6:03 PM, Yuri Gribov <tetra2005@gmail.com> wrote:
> Hi all,
>
> This is initial patch for
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57371 . Per Joseph's
> suggestion it optimizes
>   (float)lhs CMP rhs
>   (double)lhs CMP rhs
> to
>   lhs CMP (typeof(x))rhs
> whenever typeof(x) can be precisely represented by floating-point type
> (e.g. short by float or int by double) and rhs can be precisely
> represented by typeof(x).
>
> Bootstrapped/regtested on x64. Ok for trunk?

Seems my understanding of real_to_integer was too naive. I'll fix the
patch and send updated variant.

-Y

Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons

[Joseph Myers]

I'd expect much more thorough testcases here, both for cases that get 
optimized and cases that don't.  You're only testing comparisons with 
zero.  There should be comparisons with other values, both integer and 
noninteger, both within the range for which optimizing would be valid and 
outside it, both inside the range of the integer type and outside it.  
(To the extent that you don't optimize some cases that would be valid to 
optimize as discussed in that PR, XFAILed tests, or deferring adding 
tests, would be reasonable.  But each case identified in that PR as not 
valid to optimize, or only valid to optimize with -fno-trapping-math, 
should have corresponding tests that it's not optimized.)

Since SCALAR_FLOAT_TYPE_P includes decimal floating-point types, tests 
with those are desirable as well (in gcc.dg/dfp or c-c++-common/dfp, I 
suppose).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons

[Jeff Law]

On 07/03/2017 08:52 AM, Joseph Myers wrote:
> I'd expect much more thorough testcases here, both for cases that get 
> optimized and cases that don't.  You're only testing comparisons with 
> zero.  There should be comparisons with other values, both integer and 
> noninteger, both within the range for which optimizing would be valid and 
> outside it, both inside the range of the integer type and outside it.  
> (To the extent that you don't optimize some cases that would be valid to 
> optimize as discussed in that PR, XFAILed tests, or deferring adding 
> tests, would be reasonable.  But each case identified in that PR as not 
> valid to optimize, or only valid to optimize with -fno-trapping-math, 
> should have corresponding tests that it's not optimized.)
> 
> Since SCALAR_FLOAT_TYPE_P includes decimal floating-point types, tests 
> with those are desirable as well (in gcc.dg/dfp or c-c++-common/dfp, I 
> suppose).
> 
Agreed.  I think with better testing this should be able to move forward
after the technical review.  It's not terribly different conceptually
than the code in DOM/VRP, except that Yuri's changes work on floating
point types.

I'm pretty sure DOM's bits could be replaced with a suitable match.pd
pattern (which IMHO would be a small improvement across multiple axis).
VRP would be more difficult as the VRP implementation depends on getting
the value range of the RHS of the conditional.

Jeff

Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons

[Jeff Law]

On 07/02/2017 11:03 AM, Yuri Gribov wrote:
> Hi all,
> 
> This is initial patch for
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57371 . Per Joseph's
> suggestion it optimizes
>   (float)lhs CMP rhs
>   (double)lhs CMP rhs
> to
>   lhs CMP (typeof(x))rhs
> whenever typeof(x) can be precisely represented by floating-point type
> (e.g. short by float or int by double) and rhs can be precisely
> represented by typeof(x).
> 
> Bootstrapped/regtested on x64. Ok for trunk?
> 
> I'd like to extend this further in follow-up patches:
> 1) fold always-false/always-true comparisons e.g.
>   short x;
>   (float)x > INT16_MAX;  // Always false
> 2) get rid of cast in comparisons with zero regardless of typeof(lhs)
> when -fno-trapping-math:
>   (float_or_double)lhs CMP 0
> 
> -Y
> 
> 
> pr57371-1.patch
> 
> 
> 2017-07-02  Yury Gribov  <tetra2005@gmail.com>
> 
> 	PR tree-optimization/57371
> 	* match.pd: New pattern.
> 	* testsuite/gcc.dg/pr57371-1.c: New test.
> 	* testsuite/gcc.dg/pr57371-2.c: New test.
> 
> diff -rupN gcc/gcc/match.pd gcc-57371/gcc/match.pd
> --- gcc/gcc/match.pd	2017-06-29 21:14:57.000000000 +0200
> +++ gcc-57371/gcc/match.pd	2017-07-01 09:08:04.000000000 +0200
> @@ -2802,7 +2802,35 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>     (simplify
>      (cmp (sq @0) (sq @1))
>        (if (! HONOR_NANS (@0))
> -	(cmp @0 @1))))))
> +	(cmp @0 @1)))))
> +
> + /* Get rid of float cast in
> +     (float_type)N CMP M
> +    if N and M are within the range explicitly representable
> +    by float type.
> +
> +    TODO: fold always true/false comparisons if M is outside valid range.  */
> + (simplify
> +  (cmp (float @0) REAL_CST@1)
> +  (if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (@1)))
> +   (with
> +    {
> +      tree itype = TREE_TYPE (@0);
> +
> +      const real_format *fmt = REAL_MODE_FORMAT (TYPE_MODE (TREE_TYPE (@1)));
> +
> +      const REAL_VALUE_TYPE *rhs = TREE_REAL_CST_PTR (@1);
> +      bool not_rhs_int_p = false;
> +      wide_int rhs_int = real_to_integer (rhs, &not_rhs_int_p, TYPE_PRECISION (itype));
> +    }
> +    (if (!not_rhs_int_p
> +         && !(TYPE_UNSIGNED (itype) && real_isneg (rhs))
> +         && wi::ge_p (rhs_int, wi::min_value (itype), TYPE_SIGN (itype))
> +         && wi::le_p (rhs_int, wi::max_value (itype), TYPE_SIGN (itype))
> +         && TYPE_PRECISION (itype) <= significand_size (fmt))
> +     (cmp @0 { wide_int_to_tree (itype, rhs_int); })
> +    ))))
> +)
Seems like a nit, but instead of "not_rhs_int_p" use "fail" or something
like that.  That makes it easier to mentally parse the conditional which
uses the result.

What happens if @0 is a floating point type?  Based on the variable name
"itype" and passing TYPE_PRECISION (itype) to real_to_integer, it seems
like you're expecting @0 to be an integer.  If so, you should verify
that it really is an integer type.  Seems like a good thing to verify
with tests as well.

Jeff

Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons

[Yuri Gribov]

On Mon, Jul 3, 2017 at 4:38 PM, Jeff Law <law@redhat.com> wrote:
> On 07/02/2017 11:03 AM, Yuri Gribov wrote:
>> Hi all,
>>
>> This is initial patch for
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57371 . Per Joseph's
>> suggestion it optimizes
>>   (float)lhs CMP rhs
>>   (double)lhs CMP rhs
>> to
>>   lhs CMP (typeof(x))rhs
>> whenever typeof(x) can be precisely represented by floating-point type
>> (e.g. short by float or int by double) and rhs can be precisely
>> represented by typeof(x).
>>
>> Bootstrapped/regtested on x64. Ok for trunk?
>>
>> I'd like to extend this further in follow-up patches:
>> 1) fold always-false/always-true comparisons e.g.
>>   short x;
>>   (float)x > INT16_MAX;  // Always false
>> 2) get rid of cast in comparisons with zero regardless of typeof(lhs)
>> when -fno-trapping-math:
>>   (float_or_double)lhs CMP 0
>>
>> -Y
>>
>>
>> pr57371-1.patch
>>
>>
>> 2017-07-02  Yury Gribov  <tetra2005@gmail.com>
>>
>>       PR tree-optimization/57371
>>       * match.pd: New pattern.
>>       * testsuite/gcc.dg/pr57371-1.c: New test.
>>       * testsuite/gcc.dg/pr57371-2.c: New test.
>>
>> diff -rupN gcc/gcc/match.pd gcc-57371/gcc/match.pd
>> --- gcc/gcc/match.pd  2017-06-29 21:14:57.000000000 +0200
>> +++ gcc-57371/gcc/match.pd    2017-07-01 09:08:04.000000000 +0200
>> @@ -2802,7 +2802,35 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>>     (simplify
>>      (cmp (sq @0) (sq @1))
>>        (if (! HONOR_NANS (@0))
>> -     (cmp @0 @1))))))
>> +     (cmp @0 @1)))))
>> +
>> + /* Get rid of float cast in
>> +     (float_type)N CMP M
>> +    if N and M are within the range explicitly representable
>> +    by float type.
>> +
>> +    TODO: fold always true/false comparisons if M is outside valid range.  */
>> + (simplify
>> +  (cmp (float @0) REAL_CST@1)
>> +  (if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (@1)))
>> +   (with
>> +    {
>> +      tree itype = TREE_TYPE (@0);
>> +
>> +      const real_format *fmt = REAL_MODE_FORMAT (TYPE_MODE (TREE_TYPE (@1)));
>> +
>> +      const REAL_VALUE_TYPE *rhs = TREE_REAL_CST_PTR (@1);
>> +      bool not_rhs_int_p = false;
>> +      wide_int rhs_int = real_to_integer (rhs, &not_rhs_int_p, TYPE_PRECISION (itype));
>> +    }
>> +    (if (!not_rhs_int_p
>> +         && !(TYPE_UNSIGNED (itype) && real_isneg (rhs))
>> +         && wi::ge_p (rhs_int, wi::min_value (itype), TYPE_SIGN (itype))
>> +         && wi::le_p (rhs_int, wi::max_value (itype), TYPE_SIGN (itype))
>> +         && TYPE_PRECISION (itype) <= significand_size (fmt))
>> +     (cmp @0 { wide_int_to_tree (itype, rhs_int); })
>> +    ))))
>> +)
> Seems like a nit, but instead of "not_rhs_int_p" use "fail" or something
> like that.  That makes it easier to mentally parse the conditional which
> uses the result.

Actually it's even worse than that, it should actually be overflow_p
and for not_rhs_int_p I need to use other APIs.

> What happens if @0 is a floating point type?  Based on the variable name
> "itype" and passing TYPE_PRECISION (itype) to real_to_integer, it seems
> like you're expecting @0 to be an integer.  If so, you should verify
> that it really is an integer type.  Seems like a good thing to verify
> with tests as well.

Right.

-Y

Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons

[Yuri Gribov]

On Mon, Jul 3, 2017 at 4:28 PM, Jeff Law <law@redhat.com> wrote:
> On 07/03/2017 08:52 AM, Joseph Myers wrote:
>> I'd expect much more thorough testcases here, both for cases that get
>> optimized and cases that don't.  You're only testing comparisons with
>> zero.  There should be comparisons with other values, both integer and
>> noninteger, both within the range for which optimizing would be valid and
>> outside it, both inside the range of the integer type and outside it.
>> (To the extent that you don't optimize some cases that would be valid to
>> optimize as discussed in that PR, XFAILed tests, or deferring adding
>> tests, would be reasonable.  But each case identified in that PR as not
>> valid to optimize, or only valid to optimize with -fno-trapping-math,
>> should have corresponding tests that it's not optimized.)
>>
>> Since SCALAR_FLOAT_TYPE_P includes decimal floating-point types, tests
>> with those are desirable as well (in gcc.dg/dfp or c-c++-common/dfp, I
>> suppose).
>>
> Agreed.  I think with better testing this should be able to move forward
> after the technical review.  It's not terribly different conceptually
> than the code in DOM/VRP, except that Yuri's changes work on floating
> point types.
>
> I'm pretty sure DOM's bits could be replaced with a suitable match.pd
> pattern (which IMHO would be a small improvement across multiple axis).
> VRP would be more difficult as the VRP implementation depends on getting
> the value range of the RHS of the conditional.

Joseph, Jeff,

Thanks a lot for your comments. I'll work on updated version and post
it (hopefully) soon.

-Y

Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons

[Marc Glisse]

On Mon, 3 Jul 2017, Jeff Law wrote:

> On 07/02/2017 11:03 AM, Yuri Gribov wrote:
>> Hi all,
>>
>> This is initial patch for
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57371 . Per Joseph's
>> suggestion it optimizes
>>   (float)lhs CMP rhs
>>   (double)lhs CMP rhs
>> to
>>   lhs CMP (typeof(x))rhs
>> whenever typeof(x) can be precisely represented by floating-point type
>> (e.g. short by float or int by double) and rhs can be precisely
>> represented by typeof(x).
>>
>> Bootstrapped/regtested on x64. Ok for trunk?
>>
>> I'd like to extend this further in follow-up patches:
>> 1) fold always-false/always-true comparisons e.g.
>>   short x;
>>   (float)x > INT16_MAX;  // Always false
>> 2) get rid of cast in comparisons with zero regardless of typeof(lhs)
>> when -fno-trapping-math:
>>   (float_or_double)lhs CMP 0
>>
>> -Y
>>
>>
>> pr57371-1.patch
>>
>>
>> 2017-07-02  Yury Gribov  <tetra2005@gmail.com>
>>
>> 	PR tree-optimization/57371
>> 	* match.pd: New pattern.
>> 	* testsuite/gcc.dg/pr57371-1.c: New test.
>> 	* testsuite/gcc.dg/pr57371-2.c: New test.
>>
>> diff -rupN gcc/gcc/match.pd gcc-57371/gcc/match.pd
>> --- gcc/gcc/match.pd	2017-06-29 21:14:57.000000000 +0200
>> +++ gcc-57371/gcc/match.pd	2017-07-01 09:08:04.000000000 +0200
>> @@ -2802,7 +2802,35 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>>     (simplify
>>      (cmp (sq @0) (sq @1))
>>        (if (! HONOR_NANS (@0))
>> -	(cmp @0 @1))))))
>> +	(cmp @0 @1)))))
>> +
>> + /* Get rid of float cast in
>> +     (float_type)N CMP M
>> +    if N and M are within the range explicitly representable
>> +    by float type.
>> +
>> +    TODO: fold always true/false comparisons if M is outside valid range.  */
>> + (simplify
>> +  (cmp (float @0) REAL_CST@1)
>> +  (if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (@1)))
>> +   (with
>> +    {
>> +      tree itype = TREE_TYPE (@0);
>> +
>> +      const real_format *fmt = REAL_MODE_FORMAT (TYPE_MODE (TREE_TYPE (@1)));
>> +
>> +      const REAL_VALUE_TYPE *rhs = TREE_REAL_CST_PTR (@1);
>> +      bool not_rhs_int_p = false;
>> +      wide_int rhs_int = real_to_integer (rhs, &not_rhs_int_p, TYPE_PRECISION (itype));
>> +    }
>> +    (if (!not_rhs_int_p
>> +         && !(TYPE_UNSIGNED (itype) && real_isneg (rhs))
>> +         && wi::ge_p (rhs_int, wi::min_value (itype), TYPE_SIGN (itype))
>> +         && wi::le_p (rhs_int, wi::max_value (itype), TYPE_SIGN (itype))
>> +         && TYPE_PRECISION (itype) <= significand_size (fmt))
>> +     (cmp @0 { wide_int_to_tree (itype, rhs_int); })
>> +    ))))
>> +)
> Seems like a nit, but instead of "not_rhs_int_p" use "fail" or something
> like that.  That makes it easier to mentally parse the conditional which
> uses the result.
>
> What happens if @0 is a floating point type?  Based on the variable name
> "itype" and passing TYPE_PRECISION (itype) to real_to_integer, it seems
> like you're expecting @0 to be an integer.  If so, you should verify
> that it really is an integer type.  Seems like a good thing to verify
> with tests as well.

@0 is the argument of a FLOAT_EXPR. verify_gimple_assign_unary guarantees 
that it is INTEGRAL_TYPE_P (or VECTOR_INTEGER_TYPE_P but then the result 
would have to be VECTOR_FLOAT_TYPE_P, and since it gets compared to 
REAL_CST... the test SCALAR_FLOAT_TYPE_P is actually redundant).

-- 
Marc Glisse

Re: [PATCH][PR 57371] Remove useless floating point casts in comparisons

[Jeff Law]

On 07/03/2017 12:59 PM, Marc Glisse wrote:

>> What happens if @0 is a floating point type?  Based on the variable name
>> "itype" and passing TYPE_PRECISION (itype) to real_to_integer, it seems
>> like you're expecting @0 to be an integer.  If so, you should verify
>> that it really is an integer type.  Seems like a good thing to verify
>> with tests as well.
> 
> @0 is the argument of a FLOAT_EXPR. verify_gimple_assign_unary
> guarantees that it is INTEGRAL_TYPE_P (or VECTOR_INTEGER_TYPE_P but then
> the result would have to be VECTOR_FLOAT_TYPE_P, and since it gets
> compared to REAL_CST... the test SCALAR_FLOAT_TYPE_P is actually
> redundant).
Duh.  I should have realized that.  My bad.

jeff