diff mbox

Move "X +- C1 CMP C2 to X CMP C2 -+ C1" to match.pd

Message ID alpine.DEB.2.02.1604241828250.12491@laptop-mg.saclay.inria.fr
State New
Headers show

Commit Message

Marc Glisse April 24, 2016, 5:14 p.m. UTC
Hello,

trying to move a first pattern from fold_comparison.

I first tried without single_use. It brought the number of 'free' in 
g++.dg/tree-ssa/pr61034.C down to 11, changed gcc.dg/sms-6.c to only 2 SMS 
(I don't think the generated code was worse, maybe even better, but I 
don't know ppc asm), broke Wstrict-overflow-18.c (the optimization moved 
from VRP to CCP if I remember correctly), and caused IVOPTS to make a mess 
in guality/pr54693-2.c (much longer code, and many <optimized> debug 
variables). If someone wants to drop the single_use, they can work on that 
after this patch is in.

The conditions do not exactly match the ones in fold-const.c, but I guess 
they are close. The warning in the constant case was missing in 
fold_comparison, but present in VRP, so I had to add it not to regress.

I don't think we were warning much from match.pd. I can't say that I am a 
big fan of those strict overflow warnings, but I expect people would 
complain if we just dropped the existing ones when moving the transforms 
to match.pd?

I wanted to restrict the equality case to TYPE_OVERFLOW_WRAPS || 
TYPE_OVERFLOW_UNDEFINED, but that broke 20041114-1.c at -O1 (no strict 
overflow), so I went with some kind of complement we use elsewhere. Now 
that I am writing this, I don't remember why I didn't just add 
-fstrict-overflow to the testcase, or xfail it at -O1. The saturating case 
could be handled as long as the constant is not an extremum, but I don't 
think we really handle saturating integers anyway.

I split the equality case, because it was already getting ugly.

Bootstrap+regtest on powerpc64le-unknown-linux-gnu.

2016-04-25  Marc Glisse  <marc.glisse@inria.fr>

gcc/
 	* fold-const.h: Include flag-types.h.
 	(fold_overflow_warning): Declare.
 	* fold-const.c (fold_overflow_warning): Make non-static.
 	(fold_comparison): Move the transformation of X +- C1 CMP C2
 	into X CMP C2 -+ C1 ...
 	* match.pd: ... here.
 	* tree-ssa-forwprop.c (execute): Protect fold_stmt with
 	fold_defer_overflow_warnings.

gcc/testsuite/
 	* gcc.dg/tree-ssa/20040305-1.c: Adjust.

Comments

Richard Biener April 26, 2016, 11:58 a.m. UTC | #1
On Sun, Apr 24, 2016 at 7:14 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> Hello,
>
> trying to move a first pattern from fold_comparison.
>
> I first tried without single_use. It brought the number of 'free' in
> g++.dg/tree-ssa/pr61034.C down to 11, changed gcc.dg/sms-6.c to only 2 SMS
> (I don't think the generated code was worse, maybe even better, but I don't
> know ppc asm), broke Wstrict-overflow-18.c (the optimization moved from VRP
> to CCP if I remember correctly), and caused IVOPTS to make a mess in
> guality/pr54693-2.c (much longer code, and many <optimized> debug
> variables). If someone wants to drop the single_use, they can work on that
> after this patch is in.
>
> The conditions do not exactly match the ones in fold-const.c, but I guess
> they are close. The warning in the constant case was missing in
> fold_comparison, but present in VRP, so I had to add it not to regress.
>
> I don't think we were warning much from match.pd. I can't say that I am a
> big fan of those strict overflow warnings, but I expect people would
> complain if we just dropped the existing ones when moving the transforms to
> match.pd?

I just dropped them for patterns I moved (luckily we didn't have
testcases sofar ;))

If we really want to warn from match.pd then you should do the defer/undefer
stuff in fold_stmt itself (same condition I guess) and defer/undefer without
warning in gimple_fold_stmt_to_constant_1.

So you actually ran into a testcase that expected the warning?

> I wanted to restrict the equality case to TYPE_OVERFLOW_WRAPS ||
> TYPE_OVERFLOW_UNDEFINED, but that broke 20041114-1.c at -O1 (no strict
> overflow), so I went with some kind of complement we use elsewhere.

I think I prefer to move things 1:1 (unless sth regresses) and fix bugs in the
fold-const.c variant as followup (possibly also adding testcases).

IMHO -fno-strict-overflow needs to go.  It has very wary designed semantics
(ops neither wrap nor have undefined overflow).

> Now that
> I am writing this, I don't remember why I didn't just add -fstrict-overflow
> to the testcase, or xfail it at -O1. The saturating case could be handled as
> long as the constant is not an extremum, but I don't think we really handle
> saturating integers anyway.
>
> I split the equality case, because it was already getting ugly.

That's fine.

Thanks,
Richard.

> Bootstrap+regtest on powerpc64le-unknown-linux-gnu.
>
> 2016-04-25  Marc Glisse  <marc.glisse@inria.fr>
>
> gcc/
>         * fold-const.h: Include flag-types.h.
>         (fold_overflow_warning): Declare.
>         * fold-const.c (fold_overflow_warning): Make non-static.
>         (fold_comparison): Move the transformation of X +- C1 CMP C2
>         into X CMP C2 -+ C1 ...
>         * match.pd: ... here.
>         * tree-ssa-forwprop.c (execute): Protect fold_stmt with
>         fold_defer_overflow_warnings.
>
> gcc/testsuite/
>         * gcc.dg/tree-ssa/20040305-1.c: Adjust.
>
>
> --
> Marc Glisse
> Index: trunk4/gcc/fold-const.c
> ===================================================================
> --- trunk4/gcc/fold-const.c     (revision 235384)
> +++ trunk4/gcc/fold-const.c     (working copy)
> @@ -290,21 +290,21 @@ fold_undefer_and_ignore_overflow_warning
>
>  bool
>  fold_deferring_overflow_warnings_p (void)
>  {
>    return fold_deferring_overflow_warnings > 0;
>  }
>
>  /* This is called when we fold something based on the fact that signed
>     overflow is undefined.  */
>
> -static void
> +void
>  fold_overflow_warning (const char* gmsgid, enum warn_strict_overflow_code
> wc)
>  {
>    if (fold_deferring_overflow_warnings > 0)
>      {
>        if (fold_deferred_overflow_warning == NULL
>           || wc < fold_deferred_overflow_code)
>         {
>           fold_deferred_overflow_warning = gmsgid;
>           fold_deferred_overflow_code = wc;
>         }
> @@ -8366,89 +8366,20 @@ fold_comparison (location_t loc, enum tr
>  {
>    const bool equality_code = (code == EQ_EXPR || code == NE_EXPR);
>    tree arg0, arg1, tem;
>
>    arg0 = op0;
>    arg1 = op1;
>
>    STRIP_SIGN_NOPS (arg0);
>    STRIP_SIGN_NOPS (arg1);
>
> -  /* Transform comparisons of the form X +- C1 CMP C2 to X CMP C2 -+ C1.
> */
> -  if ((TREE_CODE (arg0) == PLUS_EXPR || TREE_CODE (arg0) == MINUS_EXPR)
> -      && (equality_code
> -         || (ANY_INTEGRAL_TYPE_P (TREE_TYPE (arg0))
> -             && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (arg0))))
> -      && TREE_CODE (TREE_OPERAND (arg0, 1)) == INTEGER_CST
> -      && !TREE_OVERFLOW (TREE_OPERAND (arg0, 1))
> -      && TREE_CODE (arg1) == INTEGER_CST
> -      && !TREE_OVERFLOW (arg1))
> -    {
> -      const enum tree_code
> -       reverse_op = TREE_CODE (arg0) == PLUS_EXPR ? MINUS_EXPR : PLUS_EXPR;
> -      tree const1 = TREE_OPERAND (arg0, 1);
> -      tree const2 = fold_convert_loc (loc, TREE_TYPE (const1), arg1);
> -      tree variable = TREE_OPERAND (arg0, 0);
> -      tree new_const = int_const_binop (reverse_op, const2, const1);
> -
> -      /* If the constant operation overflowed this can be
> -        simplified as a comparison against INT_MAX/INT_MIN.  */
> -      if (TREE_OVERFLOW (new_const)
> -         && !TYPE_OVERFLOW_WRAPS (TREE_TYPE (arg0)))
> -       {
> -         int const1_sgn = tree_int_cst_sgn (const1);
> -         enum tree_code code2 = code;
> -
> -         /* Get the sign of the constant on the lhs if the
> -            operation were VARIABLE + CONST1.  */
> -         if (TREE_CODE (arg0) == MINUS_EXPR)
> -           const1_sgn = -const1_sgn;
> -
> -         /* The sign of the constant determines if we overflowed
> -            INT_MAX (const1_sgn == -1) or INT_MIN (const1_sgn == 1).
> -            Canonicalize to the INT_MIN overflow by swapping the comparison
> -            if necessary.  */
> -         if (const1_sgn == -1)
> -           code2 = swap_tree_comparison (code);
> -
> -         /* We now can look at the canonicalized case
> -              VARIABLE + 1  CODE2  INT_MIN
> -            and decide on the result.  */
> -         switch (code2)
> -           {
> -           case EQ_EXPR:
> -           case LT_EXPR:
> -           case LE_EXPR:
> -             return
> -               omit_one_operand_loc (loc, type, boolean_false_node,
> variable);
> -
> -           case NE_EXPR:
> -           case GE_EXPR:
> -           case GT_EXPR:
> -             return
> -               omit_one_operand_loc (loc, type, boolean_true_node,
> variable);
> -
> -           default:
> -             gcc_unreachable ();
> -           }
> -       }
> -      else
> -       {
> -         if (!equality_code)
> -           fold_overflow_warning ("assuming signed overflow does not occur
> "
> -                                  "when changing X +- C1 cmp C2 to "
> -                                  "X cmp C2 -+ C1",
> -                                  WARN_STRICT_OVERFLOW_COMPARISON);
> -         return fold_build2_loc (loc, code, type, variable, new_const);
> -       }
> -    }
> -
>    /* For comparisons of pointers we can decompose it to a compile time
>       comparison of the base objects and the offsets into the object.
>       This requires at least one operand being an ADDR_EXPR or a
>       POINTER_PLUS_EXPR to do more than the operand_equal_p test below.  */
>    if (POINTER_TYPE_P (TREE_TYPE (arg0))
>        && (TREE_CODE (arg0) == ADDR_EXPR
>           || TREE_CODE (arg1) == ADDR_EXPR
>           || TREE_CODE (arg0) == POINTER_PLUS_EXPR
>           || TREE_CODE (arg1) == POINTER_PLUS_EXPR))
>      {
> Index: trunk4/gcc/fold-const.h
> ===================================================================
> --- trunk4/gcc/fold-const.h     (revision 235384)
> +++ trunk4/gcc/fold-const.h     (working copy)
> @@ -13,20 +13,22 @@ WARRANTY; without even the implied warra
>  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>  for more details.
>
>  You should have received a copy of the GNU General Public License
>  along with GCC; see the file COPYING3.  If not see
>  <http://www.gnu.org/licenses/>.  */
>
>  #ifndef GCC_FOLD_CONST_H
>  #define GCC_FOLD_CONST_H
>
> +#include <flag-types.h>
> +
>  /* Non-zero if we are folding constants inside an initializer; zero
>     otherwise.  */
>  extern int folding_initializer;
>
>  /* Convert between trees and native memory representation.  */
>  extern int native_encode_expr (const_tree, unsigned char *, int, int off =
> -1);
>  extern tree native_interpret_expr (tree, const unsigned char *, int);
>
>  /* Fold constants as much as possible in an expression.
>     Returns the simplified expression.
> @@ -79,20 +81,21 @@ extern bool fold_convertible_p (const_tr
>     fold_convert_loc (UNKNOWN_LOCATION, T1, T2)
>  extern tree fold_convert_loc (location_t, tree, tree);
>  extern tree fold_single_bit_test (location_t, enum tree_code, tree, tree,
> tree);
>  extern tree fold_ignored_result (tree);
>  extern tree fold_abs_const (tree, tree);
>  extern tree fold_indirect_ref_1 (location_t, tree, tree);
>  extern void fold_defer_overflow_warnings (void);
>  extern void fold_undefer_overflow_warnings (bool, const gimple *, int);
>  extern void fold_undefer_and_ignore_overflow_warnings (void);
>  extern bool fold_deferring_overflow_warnings_p (void);
> +extern void fold_overflow_warning (const char*, enum
> warn_strict_overflow_code);
>  extern int operand_equal_p (const_tree, const_tree, unsigned int);
>  extern int multiple_of_p (tree, const_tree, const_tree);
>  #define omit_one_operand(T1,T2,T3)\
>     omit_one_operand_loc (UNKNOWN_LOCATION, T1, T2, T3)
>  extern tree omit_one_operand_loc (location_t, tree, tree, tree);
>  #define omit_two_operands(T1,T2,T3,T4)\
>     omit_two_operands_loc (UNKNOWN_LOCATION, T1, T2, T3, T4)
>  extern tree omit_two_operands_loc (location_t, tree, tree, tree, tree);
>  #define invert_truthvalue(T)\
>     invert_truthvalue_loc (UNKNOWN_LOCATION, T)
> Index: trunk4/gcc/match.pd
> ===================================================================
> --- trunk4/gcc/match.pd (revision 235384)
> +++ trunk4/gcc/match.pd (working copy)
> @@ -3071,10 +3071,54 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>  (simplify
>   /* signbit(x) -> 0 if x is nonnegative.  */
>   (SIGNBIT tree_expr_nonnegative_p@0)
>   { integer_zero_node; })
>
>  (simplify
>   /* signbit(x) -> x<0 if x doesn't have signed zeros.  */
>   (SIGNBIT @0)
>   (if (!HONOR_SIGNED_ZEROS (@0))
>    (convert (lt @0 { build_real (TREE_TYPE (@0), dconst0); }))))
> +
> +/* Transform comparisons of the form X +- C1 CMP C2 to X CMP C2 -+ C1.  */
> +(for cmp (eq ne)
> + (for op (plus minus)
> +      rop (minus plus)
> +  (simplify
> +   (cmp (op@3 @0 INTEGER_CST@1) INTEGER_CST@2)
> +   (if (!TREE_OVERFLOW (@1) && !TREE_OVERFLOW (@2)
> +       && !TYPE_OVERFLOW_SANITIZED (TREE_TYPE (@0))
> +       && !TYPE_OVERFLOW_TRAPS (TREE_TYPE (@0))
> +       && !TYPE_SATURATING (TREE_TYPE (@0)))
> +    (with { tree res = int_const_binop (rop, @2, @1); }
> +     (if (TREE_OVERFLOW (res))
> +      { constant_boolean_node (cmp == NE_EXPR, type); }
> +      (if (single_use (@3))
> +       (cmp @0 { res; }))))))))
> +(for cmp (lt le gt ge)
> + (for op (plus minus)
> +      rop (minus plus)
> +  (simplify
> +   (cmp (op@3 @0 INTEGER_CST@1) INTEGER_CST@2)
> +   (if (!TREE_OVERFLOW (@1) && !TREE_OVERFLOW (@2)
> +       && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@0)))
> +    (with { tree res = int_const_binop (rop, @2, @1); }
> +     (if (TREE_OVERFLOW (res))
> +      {
> +       fold_overflow_warning (("assuming signed overflow does not occur "
> +                               "when simplifying conditional to constant"),
> +                              WARN_STRICT_OVERFLOW_CONDITIONAL);
> +        bool less = cmp == LE_EXPR || cmp == LT_EXPR;
> +       /* wi::ges_p (@2, 0) should be sufficient for a signed type.  */
> +       bool ovf_high = wi::lt_p (@1, 0, TYPE_SIGN (TREE_TYPE (@1)))
> +                       != (op == MINUS_EXPR);
> +       constant_boolean_node (less == ovf_high, type);
> +      }
> +      (if (single_use (@3))
> +       (with
> +       {
> +         fold_overflow_warning (("assuming signed overflow does not occur "
> +                                 "when changing X +- C1 cmp C2 to "
> +                                 "X cmp C2 -+ C1"),
> +                                WARN_STRICT_OVERFLOW_COMPARISON);
> +       }
> +       (cmp @0 { res; })))))))))
> Index: trunk4/gcc/testsuite/gcc.dg/tree-ssa/20040305-1.c
> ===================================================================
> --- trunk4/gcc/testsuite/gcc.dg/tree-ssa/20040305-1.c   (revision 235384)
> +++ trunk4/gcc/testsuite/gcc.dg/tree-ssa/20040305-1.c   (working copy)
> @@ -16,15 +16,15 @@ void foo(int edx, int eax)
>      }
>    if (eax == 100)
>      {
>        if (-- edx == 0)
>          afred[0] = 2;
>      }
>  }
>
>
>  /* Verify that we did a forward propagation.  */
> -/* { dg-final { scan-tree-dump-times "Replaced" 1 "forwprop1"} } */
> +/* { dg-final { scan-tree-dump-times "gimple_simplified" 1 "forwprop1"} }
> */
>
>  /* After cddce we should have two IF statements remaining as the other
>     two tests can be threaded.  */
>  /* { dg-final { scan-tree-dump-times "if " 2 "cddce2"} } */
> Index: trunk4/gcc/tree-ssa-forwprop.c
> ===================================================================
> --- trunk4/gcc/tree-ssa-forwprop.c      (revision 235384)
> +++ trunk4/gcc/tree-ssa-forwprop.c      (working copy)
> @@ -2299,37 +2299,41 @@ pass_forwprop::execute (function *fun)
>         {
>           gimple *stmt = gsi_stmt (gsi);
>           gimple *orig_stmt = stmt;
>           bool changed = false;
>           bool was_noreturn = (is_gimple_call (stmt)
>                                && gimple_call_noreturn_p (stmt));
>
>           /* Mark stmt as potentially needing revisiting.  */
>           gimple_set_plf (stmt, GF_PLF_1, false);
>
> +         fold_defer_overflow_warnings ();
>           if (fold_stmt (&gsi, fwprop_ssa_val))
>             {
>               changed = true;
>               stmt = gsi_stmt (gsi);
>               if (maybe_clean_or_replace_eh_stmt (orig_stmt, stmt))
>                 bitmap_set_bit (to_purge, bb->index);
>               if (!was_noreturn
>                   && is_gimple_call (stmt) && gimple_call_noreturn_p (stmt))
>                 to_fixup.safe_push (stmt);
>               /* Cleanup the CFG if we simplified a condition to
>                  true or false.  */
>               if (gcond *cond = dyn_cast <gcond *> (stmt))
>                 if (gimple_cond_true_p (cond)
>                     || gimple_cond_false_p (cond))
>                   cfg_changed = true;
>               update_stmt (stmt);
> +             fold_undefer_overflow_warnings (!gimple_no_warning_p (stmt),
> stmt, 0);
>             }
> +         else
> +           fold_undefer_overflow_warnings (false, NULL, 0);
>
>           switch (gimple_code (stmt))
>             {
>             case GIMPLE_ASSIGN:
>               {
>                 tree rhs1 = gimple_assign_rhs1 (stmt);
>                 enum tree_code code = gimple_assign_rhs_code (stmt);
>
>                 if (code == COND_EXPR
>                     || code == VEC_COND_EXPR)
>
Marc Glisse April 26, 2016, 8:28 p.m. UTC | #2
On Tue, 26 Apr 2016, Richard Biener wrote:

> On Sun, Apr 24, 2016 at 7:14 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
>> Hello,
>>
>> trying to move a first pattern from fold_comparison.
>>
>> I first tried without single_use. It brought the number of 'free' in
>> g++.dg/tree-ssa/pr61034.C down to 11, changed gcc.dg/sms-6.c to only 2 SMS
>> (I don't think the generated code was worse, maybe even better, but I don't
>> know ppc asm), broke Wstrict-overflow-18.c (the optimization moved from VRP
>> to CCP if I remember correctly), and caused IVOPTS to make a mess in
>> guality/pr54693-2.c (much longer code, and many <optimized> debug
>> variables). If someone wants to drop the single_use, they can work on that
>> after this patch is in.
>>
>> The conditions do not exactly match the ones in fold-const.c, but I guess
>> they are close. The warning in the constant case was missing in
>> fold_comparison, but present in VRP, so I had to add it not to regress.
>>
>> I don't think we were warning much from match.pd. I can't say that I am a
>> big fan of those strict overflow warnings, but I expect people would
>> complain if we just dropped the existing ones when moving the transforms to
>> match.pd?
>
> I just dropped them for patterns I moved (luckily we didn't have
> testcases sofar ;))
>
> If we really want to warn from match.pd then you should do the defer/undefer
> stuff in fold_stmt itself (same condition I guess) and defer/undefer without
> warning in gimple_fold_stmt_to_constant_1.

Moving it to fold_stmt_1 seems like a good idea, much better than putting 
it in forwprop. Looking at gimple_fold_stmt_to_constant_1 on the other 
hand, I have some concerns. If we do not warn for 
gimple_fold_stmt_to_constant_1, we are going to miss some warnings (I 
believe there is at least one testcase that will break, where VRP 
currently warns but CCP will come first). On the other hand, 
gimple_fold_stmt_to_constant_1 doesn't do much validation on its return 
value, each caller has their own idea of which results are acceptable, so 
it is only in the (many) callers that we can defer/undefer, or we might 
warn for transformations that we don't actually perform. CCP already has 
the defer/undefer calls around ccp_fold and thus 
gimple_fold_stmt_to_constant_1.

> So you actually ran into a testcase that expected the warning?

Several of them if I remember correctly...

>> I wanted to restrict the equality case to TYPE_OVERFLOW_WRAPS ||
>> TYPE_OVERFLOW_UNDEFINED, but that broke 20041114-1.c at -O1 (no strict
>> overflow), so I went with some kind of complement we use elsewhere.
>
> I think I prefer to move things 1:1 (unless sth regresses) and fix bugs in the
> fold-const.c variant as followup (possibly also adding testcases).

Well, yes, but things do indeed regress quite regularly when moving things 
1:1 :-( At least unless we add a big (if (GENERIC) ...) around the 
transformation.

> IMHO -fno-strict-overflow needs to go.  It has very wary designed semantics
> (ops neither wrap nor have undefined overflow).

Maybe make it an alias for -fwrapv?
Richard Biener April 27, 2016, 11:52 a.m. UTC | #3
On Tue, Apr 26, 2016 at 10:28 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Tue, 26 Apr 2016, Richard Biener wrote:
>
>> On Sun, Apr 24, 2016 at 7:14 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
>>>
>>> Hello,
>>>
>>> trying to move a first pattern from fold_comparison.
>>>
>>> I first tried without single_use. It brought the number of 'free' in
>>> g++.dg/tree-ssa/pr61034.C down to 11, changed gcc.dg/sms-6.c to only 2
>>> SMS
>>> (I don't think the generated code was worse, maybe even better, but I
>>> don't
>>> know ppc asm), broke Wstrict-overflow-18.c (the optimization moved from
>>> VRP
>>> to CCP if I remember correctly), and caused IVOPTS to make a mess in
>>> guality/pr54693-2.c (much longer code, and many <optimized> debug
>>> variables). If someone wants to drop the single_use, they can work on
>>> that
>>> after this patch is in.
>>>
>>> The conditions do not exactly match the ones in fold-const.c, but I guess
>>> they are close. The warning in the constant case was missing in
>>> fold_comparison, but present in VRP, so I had to add it not to regress.
>>>
>>> I don't think we were warning much from match.pd. I can't say that I am a
>>> big fan of those strict overflow warnings, but I expect people would
>>> complain if we just dropped the existing ones when moving the transforms
>>> to
>>> match.pd?
>>
>>
>> I just dropped them for patterns I moved (luckily we didn't have
>> testcases sofar ;))
>>
>> If we really want to warn from match.pd then you should do the
>> defer/undefer
>> stuff in fold_stmt itself (same condition I guess) and defer/undefer
>> without
>> warning in gimple_fold_stmt_to_constant_1.
>
>
> Moving it to fold_stmt_1 seems like a good idea, much better than putting it
> in forwprop. Looking at gimple_fold_stmt_to_constant_1 on the other hand, I
> have some concerns. If we do not warn for gimple_fold_stmt_to_constant_1, we
> are going to miss some warnings (I believe there is at least one testcase
> that will break, where VRP currently warns but CCP will come first). On the
> other hand, gimple_fold_stmt_to_constant_1 doesn't do much validation on its
> return value, each caller has their own idea of which results are
> acceptable, so it is only in the (many) callers that we can defer/undefer,
> or we might warn for transformations that we don't actually perform. CCP
> already has the defer/undefer calls around ccp_fold and thus
> gimple_fold_stmt_to_constant_1.

Yeah, the issue with gimple_fold_stmt_to_constant_1 is that it's usually
used in an iteration scheme and thus we may warn multiple times
and for transforms that don't end up being used...

>> So you actually ran into a testcase that expected the warning?
>
>
> Several of them if I remember correctly...

Ugh.

>>> I wanted to restrict the equality case to TYPE_OVERFLOW_WRAPS ||
>>> TYPE_OVERFLOW_UNDEFINED, but that broke 20041114-1.c at -O1 (no strict
>>> overflow), so I went with some kind of complement we use elsewhere.
>>
>>
>> I think I prefer to move things 1:1 (unless sth regresses) and fix bugs in
>> the
>> fold-const.c variant as followup (possibly also adding testcases).
>
>
> Well, yes, but things do indeed regress quite regularly when moving things
> 1:1 :-( At least unless we add a big (if (GENERIC) ...) around the
> transformation.

Sure, just wanted clarification on what changes are just (obvious) bugfixes
and what are needed to not regress in the testsuite.

>> IMHO -fno-strict-overflow needs to go.  It has very wary designed
>> semantics
>> (ops neither wrap nor have undefined overflow).
>
>
> Maybe make it an alias for -fwrapv?

Yes, for example.

Richard.

>
> --
> Marc Glisse
diff mbox

Patch

Index: trunk4/gcc/fold-const.c
===================================================================
--- trunk4/gcc/fold-const.c	(revision 235384)
+++ trunk4/gcc/fold-const.c	(working copy)
@@ -290,21 +290,21 @@  fold_undefer_and_ignore_overflow_warning
 
 bool
 fold_deferring_overflow_warnings_p (void)
 {
   return fold_deferring_overflow_warnings > 0;
 }
 
 /* This is called when we fold something based on the fact that signed
    overflow is undefined.  */
 
-static void
+void
 fold_overflow_warning (const char* gmsgid, enum warn_strict_overflow_code wc)
 {
   if (fold_deferring_overflow_warnings > 0)
     {
       if (fold_deferred_overflow_warning == NULL
 	  || wc < fold_deferred_overflow_code)
 	{
 	  fold_deferred_overflow_warning = gmsgid;
 	  fold_deferred_overflow_code = wc;
 	}
@@ -8366,89 +8366,20 @@  fold_comparison (location_t loc, enum tr
 {
   const bool equality_code = (code == EQ_EXPR || code == NE_EXPR);
   tree arg0, arg1, tem;
 
   arg0 = op0;
   arg1 = op1;
 
   STRIP_SIGN_NOPS (arg0);
   STRIP_SIGN_NOPS (arg1);
 
-  /* Transform comparisons of the form X +- C1 CMP C2 to X CMP C2 -+ C1.  */
-  if ((TREE_CODE (arg0) == PLUS_EXPR || TREE_CODE (arg0) == MINUS_EXPR)
-      && (equality_code
-	  || (ANY_INTEGRAL_TYPE_P (TREE_TYPE (arg0))
-	      && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (arg0))))
-      && TREE_CODE (TREE_OPERAND (arg0, 1)) == INTEGER_CST
-      && !TREE_OVERFLOW (TREE_OPERAND (arg0, 1))
-      && TREE_CODE (arg1) == INTEGER_CST
-      && !TREE_OVERFLOW (arg1))
-    {
-      const enum tree_code
-	reverse_op = TREE_CODE (arg0) == PLUS_EXPR ? MINUS_EXPR : PLUS_EXPR;
-      tree const1 = TREE_OPERAND (arg0, 1);
-      tree const2 = fold_convert_loc (loc, TREE_TYPE (const1), arg1);
-      tree variable = TREE_OPERAND (arg0, 0);
-      tree new_const = int_const_binop (reverse_op, const2, const1);
-
-      /* If the constant operation overflowed this can be
-	 simplified as a comparison against INT_MAX/INT_MIN.  */
-      if (TREE_OVERFLOW (new_const)
-	  && !TYPE_OVERFLOW_WRAPS (TREE_TYPE (arg0)))
-	{
-	  int const1_sgn = tree_int_cst_sgn (const1);
-	  enum tree_code code2 = code;
-
-	  /* Get the sign of the constant on the lhs if the
-	     operation were VARIABLE + CONST1.  */
-	  if (TREE_CODE (arg0) == MINUS_EXPR)
-	    const1_sgn = -const1_sgn;
-
-	  /* The sign of the constant determines if we overflowed
-	     INT_MAX (const1_sgn == -1) or INT_MIN (const1_sgn == 1).
-	     Canonicalize to the INT_MIN overflow by swapping the comparison
-	     if necessary.  */
-	  if (const1_sgn == -1)
-	    code2 = swap_tree_comparison (code);
-
-	  /* We now can look at the canonicalized case
-	       VARIABLE + 1  CODE2  INT_MIN
-	     and decide on the result.  */
-	  switch (code2)
-	    {
-	    case EQ_EXPR:
-	    case LT_EXPR:
-	    case LE_EXPR:
-	      return
-		omit_one_operand_loc (loc, type, boolean_false_node, variable);
-
-	    case NE_EXPR:
-	    case GE_EXPR:
-	    case GT_EXPR:
-	      return
-		omit_one_operand_loc (loc, type, boolean_true_node, variable);
-
-	    default:
-	      gcc_unreachable ();
-	    }
-	}
-      else
-	{
-	  if (!equality_code)
-	    fold_overflow_warning ("assuming signed overflow does not occur "
-				   "when changing X +- C1 cmp C2 to "
-				   "X cmp C2 -+ C1",
-				   WARN_STRICT_OVERFLOW_COMPARISON);
-	  return fold_build2_loc (loc, code, type, variable, new_const);
-	}
-    }
-
   /* For comparisons of pointers we can decompose it to a compile time
      comparison of the base objects and the offsets into the object.
      This requires at least one operand being an ADDR_EXPR or a
      POINTER_PLUS_EXPR to do more than the operand_equal_p test below.  */
   if (POINTER_TYPE_P (TREE_TYPE (arg0))
       && (TREE_CODE (arg0) == ADDR_EXPR
 	  || TREE_CODE (arg1) == ADDR_EXPR
 	  || TREE_CODE (arg0) == POINTER_PLUS_EXPR
 	  || TREE_CODE (arg1) == POINTER_PLUS_EXPR))
     {
Index: trunk4/gcc/fold-const.h
===================================================================
--- trunk4/gcc/fold-const.h	(revision 235384)
+++ trunk4/gcc/fold-const.h	(working copy)
@@ -13,20 +13,22 @@  WARRANTY; without even the implied warra
 FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 for more details.
 
 You should have received a copy of the GNU General Public License
 along with GCC; see the file COPYING3.  If not see
 <http://www.gnu.org/licenses/>.  */
 
 #ifndef GCC_FOLD_CONST_H
 #define GCC_FOLD_CONST_H
 
+#include <flag-types.h>
+
 /* Non-zero if we are folding constants inside an initializer; zero
    otherwise.  */
 extern int folding_initializer;
 
 /* Convert between trees and native memory representation.  */
 extern int native_encode_expr (const_tree, unsigned char *, int, int off = -1);
 extern tree native_interpret_expr (tree, const unsigned char *, int);
 
 /* Fold constants as much as possible in an expression.
    Returns the simplified expression.
@@ -79,20 +81,21 @@  extern bool fold_convertible_p (const_tr
    fold_convert_loc (UNKNOWN_LOCATION, T1, T2)
 extern tree fold_convert_loc (location_t, tree, tree);
 extern tree fold_single_bit_test (location_t, enum tree_code, tree, tree, tree);
 extern tree fold_ignored_result (tree);
 extern tree fold_abs_const (tree, tree);
 extern tree fold_indirect_ref_1 (location_t, tree, tree);
 extern void fold_defer_overflow_warnings (void);
 extern void fold_undefer_overflow_warnings (bool, const gimple *, int);
 extern void fold_undefer_and_ignore_overflow_warnings (void);
 extern bool fold_deferring_overflow_warnings_p (void);
+extern void fold_overflow_warning (const char*, enum warn_strict_overflow_code);
 extern int operand_equal_p (const_tree, const_tree, unsigned int);
 extern int multiple_of_p (tree, const_tree, const_tree);
 #define omit_one_operand(T1,T2,T3)\
    omit_one_operand_loc (UNKNOWN_LOCATION, T1, T2, T3)
 extern tree omit_one_operand_loc (location_t, tree, tree, tree);
 #define omit_two_operands(T1,T2,T3,T4)\
    omit_two_operands_loc (UNKNOWN_LOCATION, T1, T2, T3, T4)
 extern tree omit_two_operands_loc (location_t, tree, tree, tree, tree);
 #define invert_truthvalue(T)\
    invert_truthvalue_loc (UNKNOWN_LOCATION, T)
Index: trunk4/gcc/match.pd
===================================================================
--- trunk4/gcc/match.pd	(revision 235384)
+++ trunk4/gcc/match.pd	(working copy)
@@ -3071,10 +3071,54 @@  DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
 (simplify
  /* signbit(x) -> 0 if x is nonnegative.  */
  (SIGNBIT tree_expr_nonnegative_p@0)
  { integer_zero_node; })
 
 (simplify
  /* signbit(x) -> x<0 if x doesn't have signed zeros.  */
  (SIGNBIT @0)
  (if (!HONOR_SIGNED_ZEROS (@0))
   (convert (lt @0 { build_real (TREE_TYPE (@0), dconst0); }))))
+
+/* Transform comparisons of the form X +- C1 CMP C2 to X CMP C2 -+ C1.  */
+(for cmp (eq ne)
+ (for op (plus minus)
+      rop (minus plus)
+  (simplify
+   (cmp (op@3 @0 INTEGER_CST@1) INTEGER_CST@2)
+   (if (!TREE_OVERFLOW (@1) && !TREE_OVERFLOW (@2)
+	&& !TYPE_OVERFLOW_SANITIZED (TREE_TYPE (@0))
+	&& !TYPE_OVERFLOW_TRAPS (TREE_TYPE (@0))
+	&& !TYPE_SATURATING (TREE_TYPE (@0)))
+    (with { tree res = int_const_binop (rop, @2, @1); }
+     (if (TREE_OVERFLOW (res))
+      { constant_boolean_node (cmp == NE_EXPR, type); }
+      (if (single_use (@3))
+       (cmp @0 { res; }))))))))
+(for cmp (lt le gt ge)
+ (for op (plus minus)
+      rop (minus plus)
+  (simplify
+   (cmp (op@3 @0 INTEGER_CST@1) INTEGER_CST@2)
+   (if (!TREE_OVERFLOW (@1) && !TREE_OVERFLOW (@2)
+	&& TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (@0)))
+    (with { tree res = int_const_binop (rop, @2, @1); }
+     (if (TREE_OVERFLOW (res))
+      {
+	fold_overflow_warning (("assuming signed overflow does not occur "
+				"when simplifying conditional to constant"),
+			       WARN_STRICT_OVERFLOW_CONDITIONAL);
+        bool less = cmp == LE_EXPR || cmp == LT_EXPR;
+	/* wi::ges_p (@2, 0) should be sufficient for a signed type.  */
+	bool ovf_high = wi::lt_p (@1, 0, TYPE_SIGN (TREE_TYPE (@1)))
+			!= (op == MINUS_EXPR);
+	constant_boolean_node (less == ovf_high, type);
+      }
+      (if (single_use (@3))
+       (with
+	{
+	  fold_overflow_warning (("assuming signed overflow does not occur "
+				  "when changing X +- C1 cmp C2 to "
+				  "X cmp C2 -+ C1"),
+				 WARN_STRICT_OVERFLOW_COMPARISON);
+	}
+	(cmp @0 { res; })))))))))
Index: trunk4/gcc/testsuite/gcc.dg/tree-ssa/20040305-1.c
===================================================================
--- trunk4/gcc/testsuite/gcc.dg/tree-ssa/20040305-1.c	(revision 235384)
+++ trunk4/gcc/testsuite/gcc.dg/tree-ssa/20040305-1.c	(working copy)
@@ -16,15 +16,15 @@  void foo(int edx, int eax)
     }
   if (eax == 100)
     {
       if (-- edx == 0)
         afred[0] = 2;
     }
 }
  
 
 /* Verify that we did a forward propagation.  */
-/* { dg-final { scan-tree-dump-times "Replaced" 1 "forwprop1"} } */
+/* { dg-final { scan-tree-dump-times "gimple_simplified" 1 "forwprop1"} } */
 
 /* After cddce we should have two IF statements remaining as the other
    two tests can be threaded.  */
 /* { dg-final { scan-tree-dump-times "if " 2 "cddce2"} } */
Index: trunk4/gcc/tree-ssa-forwprop.c
===================================================================
--- trunk4/gcc/tree-ssa-forwprop.c	(revision 235384)
+++ trunk4/gcc/tree-ssa-forwprop.c	(working copy)
@@ -2299,37 +2299,41 @@  pass_forwprop::execute (function *fun)
 	{
 	  gimple *stmt = gsi_stmt (gsi);
 	  gimple *orig_stmt = stmt;
 	  bool changed = false;
 	  bool was_noreturn = (is_gimple_call (stmt)
 			       && gimple_call_noreturn_p (stmt));
 
 	  /* Mark stmt as potentially needing revisiting.  */
 	  gimple_set_plf (stmt, GF_PLF_1, false);
 
+	  fold_defer_overflow_warnings ();
 	  if (fold_stmt (&gsi, fwprop_ssa_val))
 	    {
 	      changed = true;
 	      stmt = gsi_stmt (gsi);
 	      if (maybe_clean_or_replace_eh_stmt (orig_stmt, stmt))
 		bitmap_set_bit (to_purge, bb->index);
 	      if (!was_noreturn
 		  && is_gimple_call (stmt) && gimple_call_noreturn_p (stmt))
 		to_fixup.safe_push (stmt);
 	      /* Cleanup the CFG if we simplified a condition to
 	         true or false.  */
 	      if (gcond *cond = dyn_cast <gcond *> (stmt))
 		if (gimple_cond_true_p (cond)
 		    || gimple_cond_false_p (cond))
 		  cfg_changed = true;
 	      update_stmt (stmt);
+	      fold_undefer_overflow_warnings (!gimple_no_warning_p (stmt), stmt, 0);
 	    }
+	  else
+	    fold_undefer_overflow_warnings (false, NULL, 0);
 
 	  switch (gimple_code (stmt))
 	    {
 	    case GIMPLE_ASSIGN:
 	      {
 		tree rhs1 = gimple_assign_rhs1 (stmt);
 		enum tree_code code = gimple_assign_rhs_code (stmt);
 
 		if (code == COND_EXPR
 		    || code == VEC_COND_EXPR)