From: Richard Biener <richard.guenther@gmail.com>
To: Alexander Monakov <amonakov@ispras.ru>
Cc: Marc Glisse <marc.glisse@inria.fr>,
GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] match.pd: reassociate multiplications with constants
Date: Wed, 19 Jul 2017 11:28:00 -0000 [thread overview]
Message-ID: <CAFiYyc0yaQ3B1VJH9B_qUZR6nUZh+BN3ccbvJdtivAef__TFgQ@mail.gmail.com> (raw)
In-Reply-To: <alpine.LNX.2.20.13.1707182003570.25203@monopod.intra.ispras.ru>
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.
> + (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" } } */
next prev parent reply other threads:[~2017-07-19 11:28 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-13 16:37 Alexander Monakov
2017-07-13 18:31 ` Marc Glisse
2017-07-13 19:42 ` Alexander Monakov
2017-07-14 6:00 ` Marc Glisse
2017-07-18 8:23 ` Richard Biener
2017-07-15 17:01 ` Alexander Monakov
2017-07-15 17:16 ` Alexander Monakov
2017-07-17 20:00 ` Marc Glisse
2017-07-17 20:50 ` Alexander Monakov
2017-07-18 17:08 ` Alexander Monakov
2017-07-19 11:28 ` Richard Biener [this message]
2017-07-19 11:33 ` Richard Biener
2017-07-19 14:40 ` Alexander Monakov
2017-07-20 8:41 ` Richard Biener
2017-07-20 9:40 ` Alexander Monakov
2017-07-20 10:51 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFiYyc0yaQ3B1VJH9B_qUZR6nUZh+BN3ccbvJdtivAef__TFgQ@mail.gmail.com \
--to=richard.guenther@gmail.com \
--cc=amonakov@ispras.ru \
--cc=gcc-patches@gcc.gnu.org \
--cc=marc.glisse@inria.fr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).