[PATCH][20/n] Remove GENERIC stmt combining from SCCVN

[PATCH][20/n] Remove GENERIC stmt combining from SCCVN

[Richard Biener]

This moves simplifying of comparisons against the highest or lowest 
possible integer.

Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.

This needs the GENERIC code-gen fix, otherwise we miscompile GCC.

Richard.

2015-07-24  Richard Biener  <rguenther@suse.de>

	* fold-const.c (fold_binary_loc): Move simplifying of comparisons
	against the highest or lowest possible integer ...
	* match.pd: ... as patterns here.

Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c	(revision 226140)
+++ gcc/fold-const.c	(working copy)
@@ -11651,123 +11463,6 @@ fold_binary_loc (location_t loc,
 	    }
 	}
 
-      /* Comparisons with the highest or lowest possible integer of
-	 the specified precision will have known values.  */
-      {
-	tree arg1_type = TREE_TYPE (arg1);
-	unsigned int prec = TYPE_PRECISION (arg1_type);
-
-	if (TREE_CODE (arg1) == INTEGER_CST
-	    && (INTEGRAL_TYPE_P (arg1_type) || POINTER_TYPE_P (arg1_type)))
-	  {
-	    wide_int max = wi::max_value (arg1_type);
-	    wide_int signed_max = wi::max_value (prec, SIGNED);
-	    wide_int min = wi::min_value (arg1_type);
-
-	    if (wi::eq_p (arg1, max))
-	      switch (code)
-		{
-		case GT_EXPR:
-		  return omit_one_operand_loc (loc, type, integer_zero_node, arg0);
-
-		case GE_EXPR:
-		  return fold_build2_loc (loc, EQ_EXPR, type, op0, op1);
-
-		case LE_EXPR:
-		  return omit_one_operand_loc (loc, type, integer_one_node, arg0);
-
-		case LT_EXPR:
-		  return fold_build2_loc (loc, NE_EXPR, type, op0, op1);
-
-		/* The GE_EXPR and LT_EXPR cases above are not normally
-		   reached because of previous transformations.  */
-
-		default:
-		  break;
-		}
-	    else if (wi::eq_p (arg1, max - 1))
-	      switch (code)
-		{
-		case GT_EXPR:
-		  arg1 = const_binop (PLUS_EXPR, arg1,
-				      build_int_cst (TREE_TYPE (arg1), 1));
-		  return fold_build2_loc (loc, EQ_EXPR, type,
-				      fold_convert_loc (loc,
-							TREE_TYPE (arg1), arg0),
-				      arg1);
-		case LE_EXPR:
-		  arg1 = const_binop (PLUS_EXPR, arg1,
-				      build_int_cst (TREE_TYPE (arg1), 1));
-		  return fold_build2_loc (loc, NE_EXPR, type,
-				      fold_convert_loc (loc, TREE_TYPE (arg1),
-							arg0),
-				      arg1);
-		default:
-		  break;
-		}
-	    else if (wi::eq_p (arg1, min))
-	      switch (code)
-		{
-		case LT_EXPR:
-		  return omit_one_operand_loc (loc, type, integer_zero_node, arg0);
-
-		case LE_EXPR:
-		  return fold_build2_loc (loc, EQ_EXPR, type, op0, op1);
-
-		case GE_EXPR:
-		  return omit_one_operand_loc (loc, type, integer_one_node, arg0);
-
-		case GT_EXPR:
-		  return fold_build2_loc (loc, NE_EXPR, type, op0, op1);
-
-		default:
-		  break;
-		}
-	    else if (wi::eq_p (arg1, min + 1))
-	      switch (code)
-		{
-		case GE_EXPR:
-		  arg1 = const_binop (MINUS_EXPR, arg1,
-				      build_int_cst (TREE_TYPE (arg1), 1));
-		  return fold_build2_loc (loc, NE_EXPR, type,
-				      fold_convert_loc (loc,
-							TREE_TYPE (arg1), arg0),
-				      arg1);
-		case LT_EXPR:
-		  arg1 = const_binop (MINUS_EXPR, arg1,
-				      build_int_cst (TREE_TYPE (arg1), 1));
-		  return fold_build2_loc (loc, EQ_EXPR, type,
-				      fold_convert_loc (loc, TREE_TYPE (arg1),
-							arg0),
-				      arg1);
-		default:
-		  break;
-		}
-
-	    else if (wi::eq_p (arg1, signed_max)
-		     && TYPE_UNSIGNED (arg1_type)
-		     /* We will flip the signedness of the comparison operator
-			associated with the mode of arg1, so the sign bit is
-			specified by this mode.  Check that arg1 is the signed
-			max associated with this sign bit.  */
-		     && prec == GET_MODE_PRECISION (TYPE_MODE (arg1_type))
-		     /* signed_type does not work on pointer types.  */
-		     && INTEGRAL_TYPE_P (arg1_type))
-	      {
-		/* The following case also applies to X < signed_max+1
-		   and X >= signed_max+1 because previous transformations.  */
-		if (code == LE_EXPR || code == GT_EXPR)
-		  {
-		    tree st = signed_type_for (arg1_type);
-		    return fold_build2_loc (loc,
-					code == LE_EXPR ? GE_EXPR : LT_EXPR,
-					type, fold_convert_loc (loc, st, arg0),
-					build_int_cst (st, 0));
-		  }
-	      }
-	  }
-      }
-
       /* If we are comparing an ABS_EXPR with a constant, we can
 	 convert all the cases into explicit comparisons, but they may
 	 well not be faster than doing the ABS and one comparison.
Index: gcc/match.pd
===================================================================
--- gcc/match.pd	(revision 226140)
+++ gcc/match.pd	(working copy)
@@ -1807,6 +1864,73 @@ (define_operator_list CBRT BUILT_IN_CBRT
    { constant_boolean_node (cmp == NE_EXPR, type); })))
 
 
+/* Non-equality compare simplifications from fold_binary  */
+(for cmp (lt gt le ge)
+ /* Comparisons with the highest or lowest possible integer of
+    the specified precision will have known values.  */
+ (simplify
+  (cmp (convert?@2 @0) INTEGER_CST@1)
+  (if ((INTEGRAL_TYPE_P (TREE_TYPE (@1)) || POINTER_TYPE_P (TREE_TYPE (@1)))
+       && tree_nop_conversion_p (TREE_TYPE (@2), TREE_TYPE (@0)))
+   (with
+    {
+      tree arg1_type = TREE_TYPE (@1);
+      unsigned int prec = TYPE_PRECISION (arg1_type);
+      wide_int max = wi::max_value (arg1_type);
+      wide_int signed_max = wi::max_value (prec, SIGNED);
+      wide_int min = wi::min_value (arg1_type);
+    }
+    (switch
+     (if (wi::eq_p (@1, max))
+      (switch
+       (if (cmp == GT_EXPR)
+	{ constant_boolean_node (false, type); })
+       (if (cmp == GE_EXPR)
+	(eq @2 @1))
+       (if (cmp == LE_EXPR)
+	{ constant_boolean_node (true, type); })
+       (if (cmp == LT_EXPR)
+	(ne @2 @1))))
+     (if (wi::eq_p (@1, max - 1))
+      (switch
+       (if (cmp == GT_EXPR)
+        (eq @2 { wide_int_to_tree (TREE_TYPE (@1), wi::add (@1, 1)); }))
+       (if (cmp == LE_EXPR)
+        (ne @2 { wide_int_to_tree (TREE_TYPE (@1), wi::add (@1, 1)); }))))
+     (if (wi::eq_p (@1, min))
+      (switch
+       (if (cmp == LT_EXPR)
+        { constant_boolean_node (false, type); })
+       (if (cmp == LE_EXPR)
+        (eq @2 @1))
+       (if (cmp == GE_EXPR)
+        { constant_boolean_node (true, type); })
+       (if (cmp == GT_EXPR)
+        (ne @2 @1))))
+     (if (wi::eq_p (@1, min + 1))
+      (switch
+       (if (cmp == GE_EXPR)
+        (ne @2 { wide_int_to_tree (TREE_TYPE (@1), wi::sub (@1, 1)); }))
+       (if (cmp == LT_EXPR)
+        (eq @2 { wide_int_to_tree (TREE_TYPE (@1), wi::sub (@1, 1)); }))))
+     (if (wi::eq_p (@1, signed_max)
+	  && TYPE_UNSIGNED (arg1_type)
+	  /* We will flip the signedness of the comparison operator
+	     associated with the mode of @1, so the sign bit is
+	     specified by this mode.  Check that @1 is the signed
+	     max associated with this sign bit.  */
+	  && prec == GET_MODE_PRECISION (TYPE_MODE (arg1_type))
+	  /* signed_type does not work on pointer types.  */
+	  && INTEGRAL_TYPE_P (arg1_type))
+      /* The following case also applies to X < signed_max+1
+	 and X >= signed_max+1 because previous transformations.  */
+      (if (cmp == LE_EXPR || cmp == GT_EXPR)
+       (with { tree st = signed_type_for (arg1_type); }
+        (if (cmp == LE_EXPR)
+	 (ge (convert:st @0) { build_zero_cst (st); })
+	 (lt (convert:st @0) { build_zero_cst (st); }))))))))))
+ 
+
 /* bool_var != 0 becomes bool_var.  */
 (simplify
  (ne @0 integer_zerop@1)

Re: [PATCH][20/n] Remove GENERIC stmt combining from SCCVN

[Eric Botcazou]

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

> 	* fold-const.c (fold_binary_loc): Move simplifying of comparisons
> 	against the highest or lowest possible integer ...
> 	* match.pd: ... as patterns here.

This incorrectly dropped the calls to omit_one_operand_loc, resulting in the 
failure of the attached Ada test: if the operand has side effects, you cannot 
replace the entire comparison with just 'true' or 'false'.


	* gnat.dg/overflow_sum3.adb: New test.

-- 
Eric Botcazou

[-- Attachment #2: overflow_sum3.adb --]
[-- Type: text/x-adasrc, Size: 340 bytes --]

--  { dg-do run }
--  { dg-options "-gnato" }

procedure Overflow_Sum3 is

   function Ident (I : Integer) return Integer is
   begin
      return I;
   end;

   X : Short_Short_Integer := Short_Short_Integer (Ident (127));

begin
   if X+1 <= 127 then
      raise Program_Error;
   end if;
exception
   when Constraint_Error => null;
end;

Re: [PATCH][20/n] Remove GENERIC stmt combining from SCCVN

[Richard Biener]

On Sat, 12 Sep 2015, Eric Botcazou wrote:

> > 	* fold-const.c (fold_binary_loc): Move simplifying of comparisons
> > 	against the highest or lowest possible integer ...
> > 	* match.pd: ... as patterns here.
> 
> This incorrectly dropped the calls to omit_one_operand_loc, resulting in the 
> failure of the attached Ada test: if the operand has side effects, you cannot 
> replace the entire comparison with just 'true' or 'false'.

Still trying to reproduce, but I suppose you hit

 /* Comparisons with the highest or lowest possible integer of
    the specified precision will have known values.  */
 (simplify
  (cmp (convert?@2 @0) INTEGER_CST@1)
  (if ((INTEGRAL_TYPE_P (TREE_TYPE (@1)) || POINTER_TYPE_P (TREE_TYPE 
(@1)))
       && tree_nop_conversion_p (TREE_TYPE (@2), TREE_TYPE (@0)))
   (with
    {
      tree arg1_type = TREE_TYPE (@1);
      unsigned int prec = TYPE_PRECISION (arg1_type);
      wide_int max = wi::max_value (arg1_type);
      wide_int signed_max = wi::max_value (prec, SIGNED);
      wide_int min = wi::min_value (arg1_type);
    }
    (switch
     (if (wi::eq_p (@1, max))
      (switch
       (if (cmp == GT_EXPR)
        { constant_boolean_node (false, type); })
       (if (cmp == GE_EXPR)
        (eq @2 @1))
       (if (cmp == LE_EXPR)
        { constant_boolean_node (true, type); })

this which should handle side-effects in @0 just fine:

/* #line 2019 "/space/rguenther/src/svn/trunk/gcc/match.pd" */
                      if (cmp == LE_EXPR)
                        {
                          if (dump_file && (dump_flags & TDF_DETAILS)) 
fprintf (dump_file, "Applying pattern match.pd:2020, %s:%d\n", __FILE__, 
__LINE__);
                          tree res;
                          res =  constant_boolean_node (true, type);
                          if (TREE_SIDE_EFFECTS (captures[0]))
                            res = build2_loc (loc, COMPOUND_EXPR, type, 
fold_ignored_result (captures[0]), res);
                          return res;

note that genmatch "inlines" omit_one_operand, so you only see
fold_ignored_result here.

So maybe the issue is with some other pattern or was latent
elsewehere.  I'll have a closer look once I manage to reproduce
the issue.

Richard.

> 
> 	* gnat.dg/overflow_sum3.adb: New test.
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH][20/n] Remove GENERIC stmt combining from SCCVN

[Richard Biener]

On Mon, 14 Sep 2015, Richard Biener wrote:

> On Sat, 12 Sep 2015, Eric Botcazou wrote:
> 
> > > 	* fold-const.c (fold_binary_loc): Move simplifying of comparisons
> > > 	against the highest or lowest possible integer ...
> > > 	* match.pd: ... as patterns here.
> > 
> > This incorrectly dropped the calls to omit_one_operand_loc, resulting in the 
> > failure of the attached Ada test: if the operand has side effects, you cannot 
> > replace the entire comparison with just 'true' or 'false'.
> 
> Still trying to reproduce, but I suppose you hit
> 
>  /* Comparisons with the highest or lowest possible integer of
>     the specified precision will have known values.  */
>  (simplify
>   (cmp (convert?@2 @0) INTEGER_CST@1)
>   (if ((INTEGRAL_TYPE_P (TREE_TYPE (@1)) || POINTER_TYPE_P (TREE_TYPE 
> (@1)))
>        && tree_nop_conversion_p (TREE_TYPE (@2), TREE_TYPE (@0)))
>    (with
>     {
>       tree arg1_type = TREE_TYPE (@1);
>       unsigned int prec = TYPE_PRECISION (arg1_type);
>       wide_int max = wi::max_value (arg1_type);
>       wide_int signed_max = wi::max_value (prec, SIGNED);
>       wide_int min = wi::min_value (arg1_type);
>     }
>     (switch
>      (if (wi::eq_p (@1, max))
>       (switch
>        (if (cmp == GT_EXPR)
>         { constant_boolean_node (false, type); })
>        (if (cmp == GE_EXPR)
>         (eq @2 @1))
>        (if (cmp == LE_EXPR)
>         { constant_boolean_node (true, type); })
> 
> this which should handle side-effects in @0 just fine:
> 
> /* #line 2019 "/space/rguenther/src/svn/trunk/gcc/match.pd" */
>                       if (cmp == LE_EXPR)
>                         {
>                           if (dump_file && (dump_flags & TDF_DETAILS)) 
> fprintf (dump_file, "Applying pattern match.pd:2020, %s:%d\n", __FILE__, 
> __LINE__);
>                           tree res;
>                           res =  constant_boolean_node (true, type);
>                           if (TREE_SIDE_EFFECTS (captures[0]))
>                             res = build2_loc (loc, COMPOUND_EXPR, type, 
> fold_ignored_result (captures[0]), res);
>                           return res;
> 
> note that genmatch "inlines" omit_one_operand, so you only see
> fold_ignored_result here.
> 
> So maybe the issue is with some other pattern or was latent
> elsewehere.  I'll have a closer look once I manage to reproduce
> the issue.

Ok, so it's folding

x == 127 ? .gnat_rcheck_CE_Overflow_Check ("overflow_sum3.adb", 14);, 0 : 
(short_short_integer) x + 1

<= 127

where op0 (the COND_EXPR) does not have TREE_SIDE_EFFECTS set but
its operand 1 has:

(gdb) p debug_tree (op0)
 <cond_expr 0x7ffff68cbf90
    type <integer_type 0x7ffff6572dc8 short_short_integer sizes-gimplified 
public visited QI
        size <integer_cst 0x7ffff68ccca8 constant visited 8>
        unit size <integer_cst 0x7ffff68cccc0 constant visited 1>
        align 8 symtab 0 alias set -1 canonical type 0x7ffff6572dc8 
precision 8 min <integer_cst 0x7ffff656a678 -128> max <integer_cst 
0x7ffff656a6c0 127> context <translation_unit_decl 0x7ffff7ff81e0 D.24> RM 
size <integer_cst 0x7ffff68ccca8 8>
        chain <type_decl 0x7ffff6900b48 short_short_integer>>
   
    arg 0 <eq_expr 0x7ffff6573938
...
    arg 1 <compound_expr 0x7ffff65739b0 type <integer_type 0x7ffff6572dc8 
short_short_integer>
        side-effects
...
    arg 2 <plus_expr 0x7ffff6573910 type <integer_type 0x7ffff6572dc8 
short_short_integer>
...

that's unexpected to the code generated by genmatch and I don't see
how omit_one_operand would handle that either.  The COND_EXPR is
originally built with TREE_SIDE_EFFECTS set but:

Hardware watchpoint 7: *$43

Old value = 65595
New value = 59
emit_check (gnu_cond=<eq_expr 0x7ffff6573938>, 
    gnu_expr=<plus_expr 0x7ffff6573910>, reason=10, gnat_node=2320)
    at /space/rguenther/src/svn/trunk/gcc/ada/gcc-interface/trans.c:8823
8823      return gnu_result;
$45 = 0

so the Ada frontend resets the flag (improperly?):

emit_check (gnu_cond=<eq_expr 0x7ffff6573938>, 
    gnu_expr=<plus_expr 0x7ffff6573910>, reason=10, gnat_node=2320)
    at /space/rguenther/src/svn/trunk/gcc/ada/gcc-interface/trans.c:8823
8823      return gnu_result;
$45 = 0
(gdb) l
8818
8819      /* GNU_RESULT has side effects if and only if GNU_EXPR has:
8820         we don't need to evaluate it just for the check.  */
8821      TREE_SIDE_EFFECTS (gnu_result) = TREE_SIDE_EFFECTS (gnu_expr);
8822
8823      return gnu_result;
8824    }


Richard.

Re: [PATCH][20/n] Remove GENERIC stmt combining from SCCVN

[Eric Botcazou]

> Still trying to reproduce, but I suppose you hit

The testcase fails as of r227729 on x86-64/Linux.

>  /* Comparisons with the highest or lowest possible integer of
>     the specified precision will have known values.  */
>  (simplify
>   (cmp (convert?@2 @0) INTEGER_CST@1)
>   (if ((INTEGRAL_TYPE_P (TREE_TYPE (@1)) || POINTER_TYPE_P (TREE_TYPE
> (@1)))
>        && tree_nop_conversion_p (TREE_TYPE (@2), TREE_TYPE (@0)))
>    (with
>     {
>       tree arg1_type = TREE_TYPE (@1);
>       unsigned int prec = TYPE_PRECISION (arg1_type);
>       wide_int max = wi::max_value (arg1_type);
>       wide_int signed_max = wi::max_value (prec, SIGNED);
>       wide_int min = wi::min_value (arg1_type);
>     }
>     (switch
>      (if (wi::eq_p (@1, max))
>       (switch
>        (if (cmp == GT_EXPR)
>         { constant_boolean_node (false, type); })
>        (if (cmp == GE_EXPR)
>         (eq @2 @1))
>        (if (cmp == LE_EXPR)
>         { constant_boolean_node (true, type); })
> 
> this which should handle side-effects in @0 just fine:
> 
> /* #line 2019 "/space/rguenther/src/svn/trunk/gcc/match.pd" */
>                       if (cmp == LE_EXPR)
>                         {
>                           if (dump_file && (dump_flags & TDF_DETAILS))
> fprintf (dump_file, "Applying pattern match.pd:2020, %s:%d\n", __FILE__,
> __LINE__);
>                           tree res;
>                           res =  constant_boolean_node (true, type);
>                           if (TREE_SIDE_EFFECTS (captures[0]))
>                             res = build2_loc (loc, COMPOUND_EXPR, type,
> fold_ignored_result (captures[0]), res);
>                           return res;
> 
> note that genmatch "inlines" omit_one_operand, so you only see
> fold_ignored_result here.

I see, then for some reason TREE_SIDE_EFFECTS is not set here.

-- 
Eric Botcazou

Re: [PATCH][20/n] Remove GENERIC stmt combining from SCCVN

[Eric Botcazou]

> Ok, so it's folding
> 
> x == 127 ? .gnat_rcheck_CE_Overflow_Check ("overflow_sum3.adb", 14);, 0 :
> (short_short_integer) x + 1
> 
> <= 127
> 
> where op0 (the COND_EXPR) does not have TREE_SIDE_EFFECTS set but
> its operand 1 has:
> 
> (gdb) p debug_tree (op0)
>  <cond_expr 0x7ffff68cbf90
>     type <integer_type 0x7ffff6572dc8 short_short_integer sizes-gimplified
> public visited QI
>         size <integer_cst 0x7ffff68ccca8 constant visited 8>
>         unit size <integer_cst 0x7ffff68cccc0 constant visited 1>
>         align 8 symtab 0 alias set -1 canonical type 0x7ffff6572dc8
> precision 8 min <integer_cst 0x7ffff656a678 -128> max <integer_cst
> 0x7ffff656a6c0 127> context <translation_unit_decl 0x7ffff7ff81e0 D.24> RM
> size <integer_cst 0x7ffff68ccca8 8>
>         chain <type_decl 0x7ffff6900b48 short_short_integer>>
> 
>     arg 0 <eq_expr 0x7ffff6573938
> ...
>     arg 1 <compound_expr 0x7ffff65739b0 type <integer_type 0x7ffff6572dc8
> short_short_integer>
>         side-effects
> ...
>     arg 2 <plus_expr 0x7ffff6573910 type <integer_type 0x7ffff6572dc8
> short_short_integer>
> ...
> 
> that's unexpected to the code generated by genmatch and I don't see
> how omit_one_operand would handle that either.

The old code was propagating the comparison inside the arms of COND_EXPR
(fold_binary_op_with_conditional_arg) before applying the transformation:

      if ((short_short_integer) x == 127 ? .gnat_rcheck_CE_Overflow_Check 
("overflow_sum3.adb", 14);, 1 : 1)

The new code does the reverse, but the old behavior can be easily restored:

Index: fold-const.c
===================================================================
--- fold-const.c        (revision 227729)
+++ fold-const.c        (working copy)
@@ -9025,10 +9025,6 @@ fold_binary_loc (location_t loc,
       && tree_swap_operands_p (arg0, arg1, true))
     return fold_build2_loc (loc, swap_tree_comparison (code), type, op1, 
op0);
 
-  tem = generic_simplify (loc, code, type, op0, op1);
-  if (tem)
-    return tem;
-
   /* ARG0 is the first operand of EXPR, and ARG1 is the second operand.
 
      First check for cases where an arithmetic operation is applied to a
@@ -9114,6 +9110,10 @@ fold_binary_loc (location_t loc,
        }
     }
 
+  tem = generic_simplify (loc, code, type, op0, op1);
+  if (tem)
+    return tem;
+
   switch (code)
     {
     case MEM_REF:

is sufficient to fix the regression.

> The COND_EXPR is originally built with TREE_SIDE_EFFECTS set but:
> 
> Hardware watchpoint 7: *$43
> 
> Old value = 65595
> New value = 59
> emit_check (gnu_cond=<eq_expr 0x7ffff6573938>,
>     gnu_expr=<plus_expr 0x7ffff6573910>, reason=10, gnat_node=2320)
>     at /space/rguenther/src/svn/trunk/gcc/ada/gcc-interface/trans.c:8823
> 8823      return gnu_result;
> $45 = 0
> 
> so the Ada frontend resets the flag (improperly?):
> 
> emit_check (gnu_cond=<eq_expr 0x7ffff6573938>,
>     gnu_expr=<plus_expr 0x7ffff6573910>, reason=10, gnat_node=2320)
>     at /space/rguenther/src/svn/trunk/gcc/ada/gcc-interface/trans.c:8823
> 8823      return gnu_result;
> $45 = 0
> (gdb) l
> 8818
> 8819      /* GNU_RESULT has side effects if and only if GNU_EXPR has:
> 8820         we don't need to evaluate it just for the check.  */
> 8821      TREE_SIDE_EFFECTS (gnu_result) = TREE_SIDE_EFFECTS (gnu_expr);
> 8822
> 8823      return gnu_result;
> 8824    }

That's old code and the comment makes it quite clear why this is done though.

-- 
Eric Botcazou

Re: [PATCH][20/n] Remove GENERIC stmt combining from SCCVN

[Richard Biener]

On Mon, 14 Sep 2015, Eric Botcazou wrote:

> > Ok, so it's folding
> > 
> > x == 127 ? .gnat_rcheck_CE_Overflow_Check ("overflow_sum3.adb", 14);, 0 :
> > (short_short_integer) x + 1
> > 
> > <= 127
> > 
> > where op0 (the COND_EXPR) does not have TREE_SIDE_EFFECTS set but
> > its operand 1 has:
> > 
> > (gdb) p debug_tree (op0)
> >  <cond_expr 0x7ffff68cbf90
> >     type <integer_type 0x7ffff6572dc8 short_short_integer sizes-gimplified
> > public visited QI
> >         size <integer_cst 0x7ffff68ccca8 constant visited 8>
> >         unit size <integer_cst 0x7ffff68cccc0 constant visited 1>
> >         align 8 symtab 0 alias set -1 canonical type 0x7ffff6572dc8
> > precision 8 min <integer_cst 0x7ffff656a678 -128> max <integer_cst
> > 0x7ffff656a6c0 127> context <translation_unit_decl 0x7ffff7ff81e0 D.24> RM
> > size <integer_cst 0x7ffff68ccca8 8>
> >         chain <type_decl 0x7ffff6900b48 short_short_integer>>
> > 
> >     arg 0 <eq_expr 0x7ffff6573938
> > ...
> >     arg 1 <compound_expr 0x7ffff65739b0 type <integer_type 0x7ffff6572dc8
> > short_short_integer>
> >         side-effects
> > ...
> >     arg 2 <plus_expr 0x7ffff6573910 type <integer_type 0x7ffff6572dc8
> > short_short_integer>
> > ...
> > 
> > that's unexpected to the code generated by genmatch and I don't see
> > how omit_one_operand would handle that either.
> 
> The old code was propagating the comparison inside the arms of COND_EXPR
> (fold_binary_op_with_conditional_arg) before applying the transformation:
> 
>       if ((short_short_integer) x == 127 ? .gnat_rcheck_CE_Overflow_Check 
> ("overflow_sum3.adb", 14);, 1 : 1)
> 
> The new code does the reverse, but the old behavior can be easily restored:
>
> Index: fold-const.c
> ===================================================================
> --- fold-const.c        (revision 227729)
> +++ fold-const.c        (working copy)
> @@ -9025,10 +9025,6 @@ fold_binary_loc (location_t loc,
>        && tree_swap_operands_p (arg0, arg1, true))
>      return fold_build2_loc (loc, swap_tree_comparison (code), type, op1, 
> op0);
>  
> -  tem = generic_simplify (loc, code, type, op0, op1);
> -  if (tem)
> -    return tem;
> -
>    /* ARG0 is the first operand of EXPR, and ARG1 is the second operand.
>  
>       First check for cases where an arithmetic operation is applied to a
> @@ -9114,6 +9110,10 @@ fold_binary_loc (location_t loc,
>         }
>      }
>  
> +  tem = generic_simplify (loc, code, type, op0, op1);
> +  if (tem)
> +    return tem;
> +
>    switch (code)
>      {
>      case MEM_REF:
> 
> is sufficient to fix the regression.

The newly generated code is better though and I can't see how we
should allow fold_binary_op_with_conditional_arg to be required
for correctness.  Iff then the "fix" would not be the above but
to move fold_binary_op_with_conditional_arg to match.pd itself.

> > The COND_EXPR is originally built with TREE_SIDE_EFFECTS set but:
> > 
> > Hardware watchpoint 7: *$43
> > 
> > Old value = 65595
> > New value = 59
> > emit_check (gnu_cond=<eq_expr 0x7ffff6573938>,
> >     gnu_expr=<plus_expr 0x7ffff6573910>, reason=10, gnat_node=2320)
> >     at /space/rguenther/src/svn/trunk/gcc/ada/gcc-interface/trans.c:8823
> > 8823      return gnu_result;
> > $45 = 0
> > 
> > so the Ada frontend resets the flag (improperly?):
> > 
> > emit_check (gnu_cond=<eq_expr 0x7ffff6573938>,
> >     gnu_expr=<plus_expr 0x7ffff6573910>, reason=10, gnat_node=2320)
> >     at /space/rguenther/src/svn/trunk/gcc/ada/gcc-interface/trans.c:8823
> > 8823      return gnu_result;
> > $45 = 0
> > (gdb) l
> > 8818
> > 8819      /* GNU_RESULT has side effects if and only if GNU_EXPR has:
> > 8820         we don't need to evaluate it just for the check.  */
> > 8821      TREE_SIDE_EFFECTS (gnu_result) = TREE_SIDE_EFFECTS (gnu_expr);
> > 8822
> > 8823      return gnu_result;
> > 8824    }
> 
> That's old code and the comment makes it quite clear why this is done though.

Yeah, but then here "we don't need to evaluate it just for the check"
applies - the check is dead code as the outer comparison is always
false.  I think what the code in the Ada frontend tries to achieve
is not actually what it does.  Or the testcase is invalid (or rather
dependent on optimization performed).

Richard.

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH][20/n] Remove GENERIC stmt combining from SCCVN

[Eric Botcazou]

> > That's old code and the comment makes it quite clear why this is done
> > though.
>
> Yeah, but then here "we don't need to evaluate it just for the check"
> applies - the check is dead code as the outer comparison is always
> false.  I think what the code in the Ada frontend tries to achieve
> is not actually what it does.  Or the testcase is invalid (or rather
> dependent on optimization performed).

I think that the testcase is valid (it is derived from a DEC Ada test written 
circa 1990).  But, yes, the quoted gigi code wants to implement the famous 
11.6 clause, whereby a check need not be raised if the expression is dead.
The problem here is that the expression drives a conditional construct with 
side effects, so it cannot be considered as dead.

In the end, the code indeed appears to be incorrect in resetting the flag.
Instead the implementation of the 11.6 clause should rely purely on the 
optimizers and -fdelete-dead-exceptions.

Thanks for your help to analyze this.

-- 
Eric Botcazou