Message ID | alpine.DEB.2.20.1604251948200.1881@laptop-mg.saclay.inria.fr |
---|---|
State | New |
Headers | show |
On Mon, Apr 25, 2016 at 9:21 PM, Marc Glisse <marc.glisse@inria.fr> wrote: > Hello, > > a simple transform to replace a more complicated one in fold-const.c. > > This patch breaks the testcase gcc.dg/gomp/loop-1.c. Indeed, the C front-end > folds too eagerly > newrhs = c_fully_fold (newrhs, false, NULL); > in build_modify_expr, and by the time the OMP code checks that the increment > in the for loop has the right form, it sees i=i*2 instead of i=i+i. Since > the original code is apparently illegal, I guess it isn't that bad... The > C++ front-end seems fine. > > Testcase no-strict-overflow-6.c also breaks. ivcanon is clever enough to > count how many iterations there are before i*=2 makes i negative, which I > guess would be great with -fwrapv, but I find it a bit suspicious with just > -fno-strict-overflow. I adjusted the testcase assuming the ivcanon was doing > the right thing (with -fstrict-overflow we generate an infinite loop > instead, so it is still testing that). > > Bootstrap+regtest on powerpc64le-unknown-linux-gnu. Ok. Note that I think /* (X /[ex] A) * A -> X. */ (simplify (mult (convert? (exact_div @0 @1)) @1) /* Look through a sign-changing conversion. */ (convert @0)) has a bug as we use operand_equal_p for comparing @1 but that treats equal but different typed INTEGER_CSTs as equal... Thus this lacks the tree_nop_conversion_p check. Thanks, Richard. > 2016-04-26 Marc Glisse <marc.glisse@inria.fr> > > gcc/ > * genmatch.c (write_predicate): Add ATTRIBUTE_UNUSED. > * fold-const.c (fold_binary_loc): Remove 2 transformations > superseded by match.pd. > * match.pd (x+x -> x*2): Generalize to integers. > > gcc/testsuite/ > * gcc.dg/fold-plusmult.c: Adjust. > * gcc.dg/no-strict-overflow-6.c: Adjust. > * gcc.dg/gomp/loop-1.c: Xfail some tests. > > -- > Marc Glisse > Index: gcc/fold-const.c > =================================================================== > --- gcc/fold-const.c (revision 235411) > +++ gcc/fold-const.c (working copy) > @@ -9949,39 +9949,20 @@ fold_binary_loc (location_t loc, > /* Transform x * -C into -x * C if x is easily negatable. */ > if (TREE_CODE (op1) == INTEGER_CST > && tree_int_cst_sgn (op1) == -1 > && negate_expr_p (op0) > && (tem = negate_expr (op1)) != op1 > && ! TREE_OVERFLOW (tem)) > return fold_build2_loc (loc, MULT_EXPR, type, > fold_convert_loc (loc, type, > negate_expr (op0)), > tem); > > - /* (A + A) * C -> A * 2 * C */ > - if (TREE_CODE (arg0) == PLUS_EXPR > - && TREE_CODE (arg1) == INTEGER_CST > - && operand_equal_p (TREE_OPERAND (arg0, 0), > - TREE_OPERAND (arg0, 1), 0)) > - return fold_build2_loc (loc, MULT_EXPR, type, > - omit_one_operand_loc (loc, type, > - TREE_OPERAND (arg0, 0), > - TREE_OPERAND (arg0, 1)), > - fold_build2_loc (loc, MULT_EXPR, type, > - build_int_cst (type, 2) , > arg1)); > - > - /* ((T) (X /[ex] C)) * C cancels out if the conversion is > - sign-changing only. */ > - if (TREE_CODE (arg1) == INTEGER_CST > - && TREE_CODE (arg0) == EXACT_DIV_EXPR > - && operand_equal_p (arg1, TREE_OPERAND (arg0, 1), 0)) > - return fold_convert_loc (loc, type, TREE_OPERAND (arg0, 0)); > - > strict_overflow_p = false; > if (TREE_CODE (arg1) == INTEGER_CST > && 0 != (tem = extract_muldiv (op0, arg1, code, NULL_TREE, > &strict_overflow_p))) > { > if (strict_overflow_p) > fold_overflow_warning (("assuming signed overflow does not " > "occur when simplifying " > "multiplication"), > WARN_STRICT_OVERFLOW_MISC); > Index: gcc/genmatch.c > =================================================================== > --- gcc/genmatch.c (revision 235411) > +++ gcc/genmatch.c (working copy) > @@ -3549,21 +3549,21 @@ decision_tree::gen (FILE *f, bool gimple > > /* Output code to implement the predicate P from the decision tree DT. */ > > void > write_predicate (FILE *f, predicate_id *p, decision_tree &dt, bool gimple) > { > fprintf (f, "\nbool\n" > "%s%s (tree t%s%s)\n" > "{\n", gimple ? "gimple_" : "tree_", p->id, > p->nargs > 0 ? ", tree *res_ops" : "", > - gimple ? ", tree (*valueize)(tree)" : ""); > + gimple ? ", tree (*valueize)(tree) ATTRIBUTE_UNUSED" : ""); > /* Conveniently make 'type' available. */ > fprintf_indent (f, 2, "tree type = TREE_TYPE (t);\n"); > > if (!gimple) > fprintf_indent (f, 2, "if (TREE_SIDE_EFFECTS (t)) return false;\n"); > dt.root->gen_kids (f, 2, gimple); > > fprintf_indent (f, 2, "return false;\n" > "}\n"); > } > Index: gcc/match.pd > =================================================================== > --- gcc/match.pd (revision 235411) > +++ gcc/match.pd (working copy) > @@ -1621,25 +1621,27 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > /* Canonicalization of binary operations. */ > > /* Convert X + -C into X - C. */ > (simplify > (plus @0 REAL_CST@1) > (if (REAL_VALUE_NEGATIVE (TREE_REAL_CST (@1))) > (with { tree tem = const_unop (NEGATE_EXPR, type, @1); } > (if (!TREE_OVERFLOW (tem) || !flag_trapping_math) > (minus @0 { tem; }))))) > > -/* Convert x+x into x*2.0. */ > +/* Convert x+x into x*2. */ > (simplify > (plus @0 @0) > (if (SCALAR_FLOAT_TYPE_P (type)) > - (mult @0 { build_real (type, dconst2); }))) > + (mult @0 { build_real (type, dconst2); }) > + (if (INTEGRAL_TYPE_P (type)) > + (mult @0 { build_int_cst (type, 2); })))) > > (simplify > (minus integer_zerop @1) > (negate @1)) > > /* (ARG0 - ARG1) is the same as (-ARG1 + ARG0). So check whether > ARG0 is zero and X + ARG0 reduces to X, since that would mean > (-ARG1 + ARG0) reduces to -ARG1. */ > (simplify > (minus real_zerop@0 @1) > Index: gcc/testsuite/gcc.dg/fold-plusmult.c > =================================================================== > --- gcc/testsuite/gcc.dg/fold-plusmult.c (revision 235411) > +++ gcc/testsuite/gcc.dg/fold-plusmult.c (working copy) > @@ -4,11 +4,11 @@ > int test1 (int a) > { > return 2*a + 2*a; > } > > int test2 (int a) > { > return (a + a)*2; > } > > -/* { dg-final { scan-tree-dump-times "<a> \\\* 4" 2 "original" } } */ > +/* { dg-final { scan-tree-dump-times "a \\\* 4" 2 "original" } } */ > Index: gcc/testsuite/gcc.dg/gomp/loop-1.c > =================================================================== > --- gcc/testsuite/gcc.dg/gomp/loop-1.c (revision 235411) > +++ gcc/testsuite/gcc.dg/gomp/loop-1.c (working copy) > @@ -37,28 +37,28 @@ f1 (int x) > ; > #pragma omp for > for (i = 5; bar (i) > i; i++) /* { dg-error "condition expression refers > to iteration variable" } */ > ; > #pragma omp for > for (i = 5; i <= baz (&i); i++) /* { dg-error "condition expression > refers to iteration variable" } */ > ; > #pragma omp for > for (i = 5; i <= i; i++) /* { dg-error "invalid controlling > predicate|condition expression refers to iteration variable" } */ > ; > - #pragma omp for /* { dg-error "increment expression refers to iteration > variable" } */ > - for (i = 5; i < 16; i += i) > + #pragma omp for /* { dg-error "increment expression refers to iteration > variable" "" { xfail *-*-* } } */ > + for (i = 5; i < 16; i += i) /* { dg-bogus "invalid increment expression" > "" { xfail *-*-* } } */ > ; > #pragma omp for > for (i = 5; i < 16; i = i + 2 * i) /* { dg-error "invalid increment > expression|increment expression refers to iteration variable" } */ > ; > - #pragma omp for /* { dg-error "increment expression refers to iteration > variable" } */ > - for (i = 5; i < 16; i = i + i) > + #pragma omp for /* { dg-error "increment expression refers to iteration > variable" "" { xfail *-*-* } } */ > + for (i = 5; i < 16; i = i + i) /* { dg-bogus "invalid increment > expression" "" { xfail *-*-* } } */ > ; > #pragma omp for > for (i = 5; i < 16; i = i + bar (i)) /* { dg-error "increment expression > refers to iteration variable" } */ > ; > #pragma omp for > for (i = 5; i < 16; i = baz (&i) + i) /* { dg-error "increment expression > refers to iteration variable" } */ > ; > #pragma omp for > for (i = 5; i < 16; i += bar (i)) /* { dg-error "increment expression > refers to iteration variable" } */ > ; > @@ -174,28 +174,28 @@ f2 (int x) > ; > #pragma omp for > for (int i = 5; bar (i) > i; i++) /* { dg-error "condition expression > refers to iteration variable" } */ > ; > #pragma omp for > for (int i = 5; i <= baz (&i); i++) /* { dg-error "condition expression > refers to iteration variable" } */ > ; > #pragma omp for > for (int i = 5; i <= i; i++) /* { dg-error "invalid controlling > predicate|condition expression refers to iteration variable" } */ > ; > - #pragma omp for /* { dg-error "increment expression refers to iteration > variable" } */ > - for (int i = 5; i < 16; i += i) > + #pragma omp for /* { dg-error "increment expression refers to iteration > variable" "" { xfail *-*-* } } */ > + for (int i = 5; i < 16; i += i) /* { dg-bogus "invalid increment > expression" "" { xfail *-*-* } } */ > ; > #pragma omp for > for (int i = 5; i < 16; i = i + 2 * i) /* { dg-error "invalid increment > expression|increment expression refers to iteration variable" } */ > ; > - #pragma omp for /* { dg-error "increment expression refers to iteration > variable" } */ > - for (int i = 5; i < 16; i = i + i) > + #pragma omp for /* { dg-error "increment expression refers to iteration > variable" "" { xfail *-*-* } } */ > + for (int i = 5; i < 16; i = i + i) /* { dg-bogus "invalid increment > expression" "" { xfail *-*-* } } */ > ; > #pragma omp for > for (int i = 5; i < 16; i = i + bar (i)) /* { dg-error "increment > expression refers to iteration variable" } */ > ; > #pragma omp for > for (int i = 5; i < 16; i = baz (&i) + i) /* { dg-error "increment > expression refers to iteration variable" } */ > ; > #pragma omp for > for (int i = 5; i < 16; i += bar (i)) /* { dg-error "increment expression > refers to iteration variable" } */ > ; > Index: gcc/testsuite/gcc.dg/no-strict-overflow-6.c > =================================================================== > --- gcc/testsuite/gcc.dg/no-strict-overflow-6.c (revision 235411) > +++ gcc/testsuite/gcc.dg/no-strict-overflow-6.c (working copy) > @@ -7,14 +7,14 @@ > strict overflow semantics. We don't test this with > -fstrict-overflow because it turns into an infinite loop. That is > OK but it would also be OK to not do that. */ > > int > foo () > { > int i, bits; > for (i = 1, bits = 1; i > 0; i += i) > ++bits; > - return bits; > + return bits - sizeof(int) * __CHAR_BIT__; > } > > -/* { dg-final { scan-tree-dump "return bits" "optimized" } } */ > +/* { dg-final { scan-tree-dump "return 0" "optimized" } } */ >
On Tue, 26 Apr 2016, Richard Biener wrote: > Note that I think > > /* (X /[ex] A) * A -> X. */ > (simplify > (mult (convert? (exact_div @0 @1)) @1) > /* Look through a sign-changing conversion. */ > (convert @0)) > > has a bug as we use operand_equal_p for comparing @1 but > that treats equal but different typed INTEGER_CSTs as equal... > > Thus this lacks the tree_nop_conversion_p check. You seemed ok with removing that check last year https://gcc.gnu.org/ml/gcc-patches/2015-05/msg01551.html For this specific pattern, I think any conversion should work. Did you have a particular example in mind?
On Tue, Apr 26, 2016 at 1:37 PM, Marc Glisse <marc.glisse@inria.fr> wrote: > On Tue, 26 Apr 2016, Richard Biener wrote: > >> Note that I think >> >> /* (X /[ex] A) * A -> X. */ >> (simplify >> (mult (convert? (exact_div @0 @1)) @1) >> /* Look through a sign-changing conversion. */ >> (convert @0)) >> >> has a bug as we use operand_equal_p for comparing @1 but >> that treats equal but different typed INTEGER_CSTs as equal... >> >> Thus this lacks the tree_nop_conversion_p check. > > > You seemed ok with removing that check last year > https://gcc.gnu.org/ml/gcc-patches/2015-05/msg01551.html Ah, ok. > For this specific pattern, I think any conversion should work. Did you have > a particular example in mind? No, I just saw the fold-const.c code again and wondered. Richard. > -- > Marc Glisse
Index: gcc/fold-const.c =================================================================== --- gcc/fold-const.c (revision 235411) +++ gcc/fold-const.c (working copy) @@ -9949,39 +9949,20 @@ fold_binary_loc (location_t loc, /* Transform x * -C into -x * C if x is easily negatable. */ if (TREE_CODE (op1) == INTEGER_CST && tree_int_cst_sgn (op1) == -1 && negate_expr_p (op0) && (tem = negate_expr (op1)) != op1 && ! TREE_OVERFLOW (tem)) return fold_build2_loc (loc, MULT_EXPR, type, fold_convert_loc (loc, type, negate_expr (op0)), tem); - /* (A + A) * C -> A * 2 * C */ - if (TREE_CODE (arg0) == PLUS_EXPR - && TREE_CODE (arg1) == INTEGER_CST - && operand_equal_p (TREE_OPERAND (arg0, 0), - TREE_OPERAND (arg0, 1), 0)) - return fold_build2_loc (loc, MULT_EXPR, type, - omit_one_operand_loc (loc, type, - TREE_OPERAND (arg0, 0), - TREE_OPERAND (arg0, 1)), - fold_build2_loc (loc, MULT_EXPR, type, - build_int_cst (type, 2) , arg1)); - - /* ((T) (X /[ex] C)) * C cancels out if the conversion is - sign-changing only. */ - if (TREE_CODE (arg1) == INTEGER_CST - && TREE_CODE (arg0) == EXACT_DIV_EXPR - && operand_equal_p (arg1, TREE_OPERAND (arg0, 1), 0)) - return fold_convert_loc (loc, type, TREE_OPERAND (arg0, 0)); - strict_overflow_p = false; if (TREE_CODE (arg1) == INTEGER_CST && 0 != (tem = extract_muldiv (op0, arg1, code, NULL_TREE, &strict_overflow_p))) { if (strict_overflow_p) fold_overflow_warning (("assuming signed overflow does not " "occur when simplifying " "multiplication"), WARN_STRICT_OVERFLOW_MISC); Index: gcc/genmatch.c =================================================================== --- gcc/genmatch.c (revision 235411) +++ gcc/genmatch.c (working copy) @@ -3549,21 +3549,21 @@ decision_tree::gen (FILE *f, bool gimple /* Output code to implement the predicate P from the decision tree DT. */ void write_predicate (FILE *f, predicate_id *p, decision_tree &dt, bool gimple) { fprintf (f, "\nbool\n" "%s%s (tree t%s%s)\n" "{\n", gimple ? "gimple_" : "tree_", p->id, p->nargs > 0 ? ", tree *res_ops" : "", - gimple ? ", tree (*valueize)(tree)" : ""); + gimple ? ", tree (*valueize)(tree) ATTRIBUTE_UNUSED" : ""); /* Conveniently make 'type' available. */ fprintf_indent (f, 2, "tree type = TREE_TYPE (t);\n"); if (!gimple) fprintf_indent (f, 2, "if (TREE_SIDE_EFFECTS (t)) return false;\n"); dt.root->gen_kids (f, 2, gimple); fprintf_indent (f, 2, "return false;\n" "}\n"); } Index: gcc/match.pd =================================================================== --- gcc/match.pd (revision 235411) +++ gcc/match.pd (working copy) @@ -1621,25 +1621,27 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) /* Canonicalization of binary operations. */ /* Convert X + -C into X - C. */ (simplify (plus @0 REAL_CST@1) (if (REAL_VALUE_NEGATIVE (TREE_REAL_CST (@1))) (with { tree tem = const_unop (NEGATE_EXPR, type, @1); } (if (!TREE_OVERFLOW (tem) || !flag_trapping_math) (minus @0 { tem; }))))) -/* Convert x+x into x*2.0. */ +/* Convert x+x into x*2. */ (simplify (plus @0 @0) (if (SCALAR_FLOAT_TYPE_P (type)) - (mult @0 { build_real (type, dconst2); }))) + (mult @0 { build_real (type, dconst2); }) + (if (INTEGRAL_TYPE_P (type)) + (mult @0 { build_int_cst (type, 2); })))) (simplify (minus integer_zerop @1) (negate @1)) /* (ARG0 - ARG1) is the same as (-ARG1 + ARG0). So check whether ARG0 is zero and X + ARG0 reduces to X, since that would mean (-ARG1 + ARG0) reduces to -ARG1. */ (simplify (minus real_zerop@0 @1) Index: gcc/testsuite/gcc.dg/fold-plusmult.c =================================================================== --- gcc/testsuite/gcc.dg/fold-plusmult.c (revision 235411) +++ gcc/testsuite/gcc.dg/fold-plusmult.c (working copy) @@ -4,11 +4,11 @@ int test1 (int a) { return 2*a + 2*a; } int test2 (int a) { return (a + a)*2; } -/* { dg-final { scan-tree-dump-times "<a> \\\* 4" 2 "original" } } */ +/* { dg-final { scan-tree-dump-times "a \\\* 4" 2 "original" } } */ Index: gcc/testsuite/gcc.dg/gomp/loop-1.c =================================================================== --- gcc/testsuite/gcc.dg/gomp/loop-1.c (revision 235411) +++ gcc/testsuite/gcc.dg/gomp/loop-1.c (working copy) @@ -37,28 +37,28 @@ f1 (int x) ; #pragma omp for for (i = 5; bar (i) > i; i++) /* { dg-error "condition expression refers to iteration variable" } */ ; #pragma omp for for (i = 5; i <= baz (&i); i++) /* { dg-error "condition expression refers to iteration variable" } */ ; #pragma omp for for (i = 5; i <= i; i++) /* { dg-error "invalid controlling predicate|condition expression refers to iteration variable" } */ ; - #pragma omp for /* { dg-error "increment expression refers to iteration variable" } */ - for (i = 5; i < 16; i += i) + #pragma omp for /* { dg-error "increment expression refers to iteration variable" "" { xfail *-*-* } } */ + for (i = 5; i < 16; i += i) /* { dg-bogus "invalid increment expression" "" { xfail *-*-* } } */ ; #pragma omp for for (i = 5; i < 16; i = i + 2 * i) /* { dg-error "invalid increment expression|increment expression refers to iteration variable" } */ ; - #pragma omp for /* { dg-error "increment expression refers to iteration variable" } */ - for (i = 5; i < 16; i = i + i) + #pragma omp for /* { dg-error "increment expression refers to iteration variable" "" { xfail *-*-* } } */ + for (i = 5; i < 16; i = i + i) /* { dg-bogus "invalid increment expression" "" { xfail *-*-* } } */ ; #pragma omp for for (i = 5; i < 16; i = i + bar (i)) /* { dg-error "increment expression refers to iteration variable" } */ ; #pragma omp for for (i = 5; i < 16; i = baz (&i) + i) /* { dg-error "increment expression refers to iteration variable" } */ ; #pragma omp for for (i = 5; i < 16; i += bar (i)) /* { dg-error "increment expression refers to iteration variable" } */ ; @@ -174,28 +174,28 @@ f2 (int x) ; #pragma omp for for (int i = 5; bar (i) > i; i++) /* { dg-error "condition expression refers to iteration variable" } */ ; #pragma omp for for (int i = 5; i <= baz (&i); i++) /* { dg-error "condition expression refers to iteration variable" } */ ; #pragma omp for for (int i = 5; i <= i; i++) /* { dg-error "invalid controlling predicate|condition expression refers to iteration variable" } */ ; - #pragma omp for /* { dg-error "increment expression refers to iteration variable" } */ - for (int i = 5; i < 16; i += i) + #pragma omp for /* { dg-error "increment expression refers to iteration variable" "" { xfail *-*-* } } */ + for (int i = 5; i < 16; i += i) /* { dg-bogus "invalid increment expression" "" { xfail *-*-* } } */ ; #pragma omp for for (int i = 5; i < 16; i = i + 2 * i) /* { dg-error "invalid increment expression|increment expression refers to iteration variable" } */ ; - #pragma omp for /* { dg-error "increment expression refers to iteration variable" } */ - for (int i = 5; i < 16; i = i + i) + #pragma omp for /* { dg-error "increment expression refers to iteration variable" "" { xfail *-*-* } } */ + for (int i = 5; i < 16; i = i + i) /* { dg-bogus "invalid increment expression" "" { xfail *-*-* } } */ ; #pragma omp for for (int i = 5; i < 16; i = i + bar (i)) /* { dg-error "increment expression refers to iteration variable" } */ ; #pragma omp for for (int i = 5; i < 16; i = baz (&i) + i) /* { dg-error "increment expression refers to iteration variable" } */ ; #pragma omp for for (int i = 5; i < 16; i += bar (i)) /* { dg-error "increment expression refers to iteration variable" } */ ; Index: gcc/testsuite/gcc.dg/no-strict-overflow-6.c =================================================================== --- gcc/testsuite/gcc.dg/no-strict-overflow-6.c (revision 235411) +++ gcc/testsuite/gcc.dg/no-strict-overflow-6.c (working copy) @@ -7,14 +7,14 @@ strict overflow semantics. We don't test this with -fstrict-overflow because it turns into an infinite loop. That is OK but it would also be OK to not do that. */ int foo () { int i, bits; for (i = 1, bits = 1; i > 0; i += i) ++bits; - return bits; + return bits - sizeof(int) * __CHAR_BIT__; } -/* { dg-final { scan-tree-dump "return bits" "optimized" } } */ +/* { dg-final { scan-tree-dump "return 0" "optimized" } } */