Re: [PATCH] match.pd: reassociate multiplications with constants

[Richard Biener <richard.guenther@gmail.com>]

On Wed, Jul 19, 2017 at 1:28 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Tue, Jul 18, 2017 at 7:07 PM, Alexander Monakov <amonakov@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" } } */