On Wed, Jul 19, 2017 at 1:28 PM, Richard Biener <richard.guent...@gmail.com> wrote: > On Tue, Jul 18, 2017 at 7:07 PM, Alexander Monakov <amona...@ispras.ru> wrote: >> On Mon, 17 Jul 2017, Alexander Monakov wrote: >>> On Mon, 17 Jul 2017, Marc Glisse wrote: >>> > > +/* Combine successive multiplications. Similar to above, but handling >>> > > + overflow is different. */ >>> > > +(simplify >>> > > + (mult (mult @0 INTEGER_CST@1) INTEGER_CST@2) >>> > > + (with { >>> > > + bool overflow_p; >>> > > + wide_int mul = wi::mul (@1, @2, TYPE_SIGN (type), &overflow_p); >>> > > + } >>> > > + (if (!overflow_p || TYPE_OVERFLOW_WRAPS (type)) >>> > >>> > I wonder if there are cases where this would cause trouble for saturating >>> > integers. The only case I can think of is when @2 is -1, but that's likely >>> > simplified to NEGATE_EXPR first. >>> >>> Ah, yes, I think if @2 is -1 or 0 then we should not attempt this transform >>> for >>> either saturating or sanitized types, just like in the first patch. I think >>> wrapping the 'with' with 'if (!integer_minus_onep (@2) && !integer_zerop >>> (@2))' >>> works, since as you say it should become a negate/zero anyway? >> >> Updated patch: >> >> * match.pd ((X * CST1) * CST2): Simplify to X * (CST1 * CST2). >> testsuite: >> * gcc.dg/tree-ssa/assoc-2.c: Enhance. >> * gcc.dg/tree-ssa/slsr-4.c: Adjust. >> >> diff --git a/gcc/match.pd b/gcc/match.pd >> index 36045f1..0bb5541 100644 >> --- a/gcc/match.pd >> +++ b/gcc/match.pd >> @@ -283,6 +283,20 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) >> || mul != wi::min_value (TYPE_PRECISION (type), SIGNED)) >> { build_zero_cst (type); }))))) >> >> +/* Combine successive multiplications. Similar to above, but handling >> + overflow is different. */ >> +(simplify >> + (mult (mult @0 INTEGER_CST@1) INTEGER_CST@2) >> + /* More specific rules can handle 0 and -1; skip them here to avoid >> + wrong transformations for sanitized and saturating types. */ >> + (if (!integer_zerop (@2) && !integer_minus_onep (@2)) > > I think integer_zerop (@2) should never happen here if you order the > pattern after > > (simplify > (mult @0 integer_zerop@1) > @1) > > which I think you do. Why's @1 == -1 ok when sanitizing but not @2 == -1? > If you rely on > > /* Transform x * -1 into -x. */ > (simplify > (mult @0 integer_minus_onep) > (negate @0)) > > then you need to move that pattern above yours (there seem to be a bunch > of related ones like 0 - @1 -> -@1 to move earlier as well). > > That would leave us with the case of saturating types (the above transforms > doesn't care for those). > > So unless you can come up with a testcase that breaks I'd remove the > integer_zerop/integer_minus_onep checks.
So for saturating types isn't the issue when @1 and @2 have opposite sign and the inner multiply would have saturated? [how do saturated types behave with sign-changing multiplication/negation anyway?] >> + (with { >> + bool overflow_p; >> + wide_int mul = wi::mul (@1, @2, TYPE_SIGN (type), &overflow_p); >> + } >> + (if (!overflow_p || TYPE_OVERFLOW_WRAPS (type)) > > I think overflow in the constant multiplication is ok unless > TYPE_OVERFLOW_SANITIZED > (and can we have a ubsan testcase for that that would otherwise fail?). > It's not that we introduce new overflow here. > > Ok with those changes. > > Thanks, > Richard. > >> + (mult @0 { wide_int_to_tree (type, mul); }))))) >> + >> /* Optimize A / A to 1.0 if we don't care about >> NaNs or Infinities. */ >> (simplify >> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/assoc-2.c >> b/gcc/testsuite/gcc.dg/tree-ssa/assoc-2.c >> index a92c882..cc0e9d4 100644 >> --- a/gcc/testsuite/gcc.dg/tree-ssa/assoc-2.c >> +++ b/gcc/testsuite/gcc.dg/tree-ssa/assoc-2.c >> @@ -5,4 +5,15 @@ int f0(int a, int b){ >> return a * 33 * b * 55; >> } >> >> -/* { dg-final { scan-tree-dump-times "mult_expr" 2 "gimple" } } */ >> +int f1(int a){ >> + a *= 33; >> + return a * 55; >> +} >> + >> +int f2(int a, int b){ >> + a *= 33; >> + return a * b * 55; >> +} >> + >> +/* { dg-final { scan-tree-dump-times "mult_expr" 7 "gimple" } } */ >> +/* { dg-final { scan-tree-dump-times "mult_expr" 5 "optimized" } } */ >> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/slsr-4.c >> b/gcc/testsuite/gcc.dg/tree-ssa/slsr-4.c >> index 17d7b4c..1e943b7 100644 >> --- a/gcc/testsuite/gcc.dg/tree-ssa/slsr-4.c >> +++ b/gcc/testsuite/gcc.dg/tree-ssa/slsr-4.c >> @@ -23,13 +23,9 @@ f (int i) >> foo (y); >> } >> >> -/* { dg-final { scan-tree-dump-times "\\* 4" 1 "slsr" } } */ >> -/* { dg-final { scan-tree-dump-times "\\* 10" 1 "slsr" } } */ >> -/* { dg-final { scan-tree-dump-times "\\+ 20;" 1 "slsr" } } */ >> +/* { dg-final { scan-tree-dump-times "\\* 40" 1 "slsr" } } */ >> /* { dg-final { scan-tree-dump-times "\\+ 200" 1 "slsr" } } */ >> -/* { dg-final { scan-tree-dump-times "\\- 16;" 1 "slsr" } } */ >> /* { dg-final { scan-tree-dump-times "\\- 160" 1 "slsr" } } */ >> -/* { dg-final { scan-tree-dump-times "\\* 4" 1 "optimized" } } */ >> -/* { dg-final { scan-tree-dump-times "\\* 10" 1 "optimized" } } */ >> +/* { dg-final { scan-tree-dump-times "\\* 40" 1 "optimized" } } */ >> /* { dg-final { scan-tree-dump-times "\\+ 200" 1 "optimized" } } */ >> /* { dg-final { scan-tree-dump-times "\\+ 40" 1 "optimized" } } */