Fix floating point handling in dom (PR tree-optimization/84235)

Hi!

As the following testcase shows, dom2 miscompiles floating point x - x
into 0.0 even when x could be infinity and x - x then a NaN.
The corresponding match.pd optimization is:
/* Simplify x - x.
   This is unsafe for certain floats even in non-IEEE formats.
   In IEEE, it is unsafe because it does wrong for NaNs.
   Also note that operand_equal_p is always false if an operand
   is volatile.  */
(simplify
 (minus @0 @0)
 (if (!FLOAT_TYPE_P (type) || !HONOR_NANS (type))
  { build_zero_cst (type); }))
The patch makes it match what match.pd does.
We also have:
 /* X / X is one.  */
 (simplify
  (div @0 @0)
  /* But not for 0 / 0 so that we can get the proper warnings and errors.
     And not for _Fract types where we can't build 1.  */
  (if (!integer_zerop (@0) && !ALL_FRACT_MODE_P (TYPE_MODE (type)))
   { build_one_cst (type); }))
We can ignore the 0 / 0 case, we have both operands SSA_NAMEs and match.pd
only avoids optimizing away literal 0 / 0, but the rest is valid, some fract
types don't have a way to express 1.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2018-02-06  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/84235
	* tree-ssa-scopedtables.c
	(avail_exprs_stack::simplify_binary_operation): Fir MINUS_EXPR, punt
	if the subtraction is performed in floating point type where NaNs are
	honored.  For *DIV_EXPR, punt for ALL_FRACT_MODE_Ps where we can't
	build 1.  Formatting fix.

	* gcc.c-torture/execute/ieee/pr84235.c: New test.


	Jakub

On February 6, 2018 9:40:37 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>As the following testcase shows, dom2 miscompiles floating point x - x
>into 0.0 even when x could be infinity and x - x then a NaN.
>The corresponding match.pd optimization is:
>/* Simplify x - x.
>   This is unsafe for certain floats even in non-IEEE formats.
>   In IEEE, it is unsafe because it does wrong for NaNs.
>   Also note that operand_equal_p is always false if an operand
>   is volatile.  */
>(simplify
> (minus @0 @0)
> (if (!FLOAT_TYPE_P (type) || !HONOR_NANS (type))
>  { build_zero_cst (type); }))
>The patch makes it match what match.pd does.
>We also have:
> /* X / X is one.  */
> (simplify
>  (div @0 @0)
>/* But not for 0 / 0 so that we can get the proper warnings and errors.
>     And not for _Fract types where we can't build 1.  */
>  (if (!integer_zerop (@0) && !ALL_FRACT_MODE_P (TYPE_MODE (type)))
>   { build_one_cst (type); }))
>We can ignore the 0 / 0 case, we have both operands SSA_NAMEs and
>match.pd
>only avoids optimizing away literal 0 / 0, but the rest is valid, some
>fract
>types don't have a way to express 1.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK. 

I wonder why we have to re-implement all this in DOM. there's enough of match and simplify interfaces to make it use that? 

Richard. 


>2018-02-06  Jakub Jelinek  <jakub@redhat.com>
>
>	PR tree-optimization/84235
>	* tree-ssa-scopedtables.c
>	(avail_exprs_stack::simplify_binary_operation): Fir MINUS_EXPR, punt
>	if the subtraction is performed in floating point type where NaNs are
>	honored.  For *DIV_EXPR, punt for ALL_FRACT_MODE_Ps where we can't
>	build 1.  Formatting fix.
>
>	* gcc.c-torture/execute/ieee/pr84235.c: New test.
>
>--- gcc/tree-ssa-scopedtables.c.jj	2018-01-03 10:19:54.528533857 +0100
>+++ gcc/tree-ssa-scopedtables.c	2018-02-06 14:58:08.944673984 +0100
>@@ -182,8 +182,15 @@ avail_exprs_stack::simplify_binary_opera
> 		      case BIT_AND_EXPR:
> 			return gimple_assign_rhs1 (stmt);
> 
>-		      case BIT_XOR_EXPR:
> 		      case MINUS_EXPR:
>+			/* This is unsafe for certain floats even in non-IEEE
>+			   formats.  In IEEE, it is unsafe because it does
>+			   wrong for NaNs.  */
>+			if (FLOAT_TYPE_P (result_type)
>+			    && HONOR_NANS (result_type))
>+			  break;
>+			/* FALLTHRU */
>+		      case BIT_XOR_EXPR:
> 		      case TRUNC_MOD_EXPR:
> 		      case CEIL_MOD_EXPR:
> 		      case FLOOR_MOD_EXPR:
>@@ -195,6 +202,9 @@ avail_exprs_stack::simplify_binary_opera
> 		      case FLOOR_DIV_EXPR:
> 		      case ROUND_DIV_EXPR:
> 		      case EXACT_DIV_EXPR:
>+			/* Avoid _Fract types where we can't build 1.  */
>+			if (ALL_FRACT_MODE_P (TYPE_MODE (result_type)))
>+			  break;
> 			return build_one_cst (result_type);
> 
> 		      default:
>@@ -204,8 +214,8 @@ avail_exprs_stack::simplify_binary_opera
> 		break;
> 	      }
> 
>-	      default:
>-		break;
>+	    default:
>+	      break;
> 	    }
> 	}
>     }
>--- gcc/testsuite/gcc.c-torture/execute/ieee/pr84235.c.jj	2018-02-06
>15:04:26.528454766 +0100
>+++ gcc/testsuite/gcc.c-torture/execute/ieee/pr84235.c	2018-02-06
>15:05:06.836341334 +0100
>@@ -0,0 +1,11 @@
>+/* PR tree-optimization/84235 */
>+
>+int
>+main ()
>+{
>+  double d = 1.0 / 0.0;
>+  _Bool b = d == d && (d - d) != (d - d);
>+  if (!b)
>+    __builtin_abort ();
>+  return 0;
>+}
>
>	Jakub

On Wed, Feb 07, 2018 at 08:06:53AM +0100, Richard Biener wrote:
> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> OK. 

Committed, thanks.

> I wonder why we have to re-implement all this in DOM.  there's enough of
> match and simplify interfaces to make it use that?

It would certainly be nicer to be able to just use match.pd here.
I guess the intent is only do it if it folds into a constant.  While we
could use a valueize hook that would return the same SSA_NAME for both
values, wonder if gimplify-match.c has some API to create stmts in a
sequence only instead of replacing the stmt(s) in the IL that we could use,
test if it is a constant and use it, otherwise throw away.

	Jakub

On Wed, 7 Feb 2018, Jakub Jelinek wrote:

> On Wed, Feb 07, 2018 at 08:06:53AM +0100, Richard Biener wrote:
> > >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > 
> > OK. 
> 
> Committed, thanks.
> 
> > I wonder why we have to re-implement all this in DOM.  there's enough of
> > match and simplify interfaces to make it use that?
> 
> It would certainly be nicer to be able to just use match.pd here.
> I guess the intent is only do it if it folds into a constant.  While we
> could use a valueize hook that would return the same SSA_NAME for both
> values, wonder if gimplify-match.c has some API to create stmts in a
> sequence only instead of replacing the stmt(s) in the IL that we could use,
> test if it is a constant and use it, otherwise throw away.

Yes it does.  SCCVN for example uses it - not 100% "nice" but you
can look at vn_nary_build_or_lookup_1 for example.  The interface
is using gimple_resimplify[123], passing a NULL gimple_seq (never
allow any extra stmts as simplification) and expanded ops/code.
The simplification result is in the same expanded form and you
can use gimple_simplified_result_is_gimple_val to check if it
is a singleton and then is_gimple_min_invariant if it is constant.
But maybe we can also allow SSA names as result, like from
a - b + b or so.

There's of course the gimple_fold_stmt_to_constant[_1] API that
you can also use on existing gimple stmts which will return
a is_gimple_val as well (and will valueize things in the _1 variant
with the appropriate callbacks).  Not sure if everything DOM
wants to simplify is readily available in the IL though.  If not,
the above interface is the correct thing to use.

Well - or simply use fold_{unary,binary} ...

Richard.

On 02/07/2018 12:06 AM, Richard Biener wrote:
> On February 6, 2018 9:40:37 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
>> Hi!
>>
>> As the following testcase shows, dom2 miscompiles floating point x - x
>> into 0.0 even when x could be infinity and x - x then a NaN.
>> The corresponding match.pd optimization is:
>> /* Simplify x - x.
>>   This is unsafe for certain floats even in non-IEEE formats.
>>   In IEEE, it is unsafe because it does wrong for NaNs.
>>   Also note that operand_equal_p is always false if an operand
>>   is volatile.  */
>> (simplify
>> (minus @0 @0)
>> (if (!FLOAT_TYPE_P (type) || !HONOR_NANS (type))
>>  { build_zero_cst (type); }))
>> The patch makes it match what match.pd does.
>> We also have:
>> /* X / X is one.  */
>> (simplify
>>  (div @0 @0)
>> /* But not for 0 / 0 so that we can get the proper warnings and errors.
>>     And not for _Fract types where we can't build 1.  */
>>  (if (!integer_zerop (@0) && !ALL_FRACT_MODE_P (TYPE_MODE (type)))
>>   { build_one_cst (type); }))
>> We can ignore the 0 / 0 case, we have both operands SSA_NAMEs and
>> match.pd
>> only avoids optimizing away literal 0 / 0, but the rest is valid, some
>> fract
>> types don't have a way to express 1.
>>
>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> OK. 
> 
> I wonder why we have to re-implement all this in DOM. there's enough of match and simplify interfaces to make it use that? 
What's going on with this code is we have some kind of binary
expression.  We do not know the value of either operand.

However, we have recorded that the two operands are equal to each other
via a conditional equivalence.  ie, we have 1 = (A == B) in the
expression hash table.

For many binary operations knowing the operands are equal allows us to
eliminate the binary operation.

This was part of avoiding regressing when we stopped recording
conditional equivalences in the cprop tables.

What I'm concerned about here is how did we get into this code for a
floating point object  That should have been filtered out earlier!


Jeff