[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)