On Sun, Apr 24, 2016 at 7:14 PM, Marc Glisse <marc.gli...@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.gli...@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)
>
