From: Richard Biener <richard.guenther@gmail.com>
To: Marc Glisse <marc.glisse@inria.fr>
Cc: "Hurugalawadi, Naveen" <Naveen.Hurugalawadi@caviumnetworks.com>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
Subject: Re: Move some bit and binary optimizations in simplify and match
Date: Wed, 21 Oct 2015 09:49:00 -0000 [thread overview]
Message-ID: <CAFiYyc2XZfLKBRJj1ynpRgVpiWj6fMhp9s1YRJAASSaYe=-GYA@mail.gmail.com> (raw)
In-Reply-To: <alpine.DEB.2.20.1510210843060.2154@laptop-mg.saclay.inria.fr>
On Wed, Oct 21, 2015 at 8:48 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Tue, 20 Oct 2015, Richard Biener wrote:
>
>> On Tue, Oct 20, 2015 at 8:46 AM, Hurugalawadi, Naveen
>> <Naveen.Hurugalawadi@caviumnetworks.com> wrote:
>>>
>>> Hi,
>>>
>>>>> +/* Fold X + (X / CST) * -CST to X % CST. */
>>>>> This one is still wrong
>>>
>>> Removed.
>>>
>>>>> I don't understand the point of the FLOAT_TYPE_P check.
>>>
>>> The check was there in fold-const. So, just had the same check.
>>>
>>>>> Will we also simplify (A & B) - (A & ~B) into B - (A ^ B) ?
>>>
>>> Done.
>>>
>>>>> or maybe integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1))
>>>
>>> Done.
>>>
>>>>> :c on bit_ior? It should also allow you to merge the 2 CST versions
>>>>> into one.
>>>
>>> Had it. But due to the error on "logical_inverted_value"; confused it
>>> with
>>> this pattern and had duplicated it.
>>>
>>>>> I am not really in favor of the indentation change (or the comment).
>>>
>>> Done.
>>>
>>>>> This patch is ok when bootstrapped / tested and with a proper changelog
>>>>> entry.
>>>
>>> Regression tested on X86_64 with no extra failures.
>>
>>
>> +(simplify
>> + (minus (bit_and:s @0 INTEGER_CST@2) (bit_and:s @0 INTEGER_CST@1))
>> + (if (integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1)))
>> + (minus (bit_xor @0 @1) @1)))
>>
>> use if (wi::bit_and (@2, @1) == 0)
>
>
> I believe the original transformation required @1 and @2 to be bit_not
> of each other, so this might not work.
Oh, sorry - indeed. It fails for @2 == @1 == 0. So using bit_xor
is required or simply wi::bit_not (@2) == @1.
> He had a suitable test using wi,
> it is my fault he changed it (sorry for not being clear enough that my
> remark wasn't meant as a patch). The reason was so we could change
> INTEGER_CST to CONSTANT_CLASS_P and handle vectors.
I realized that but until that happens we should stick with the faster and
simpler wi:: interface. The tree interface will allocate extra garbage
for computing the BIT_XOR_EXPR even when no transform is done
thus I believe we'd want a integer_bit_not_of_the_other () function
avoiding extra tree garbage in the end.
>> and instead of the 2nd group
>>
>> +/* Fold (A & B) - (A & ~B) into B - (A ^ B). */
>> +(simplify
>> + (minus (bit_and:s @0 @1) (bit_and:s @0 (bit_not @1)))
>> + (minus @1 (bit_xor @0 @1)))
>> +(simplify
>> + (minus (bit_and:s @0 INTEGER_CST@1) (bit_and:s @0 INTEGER_CST@2))
>> + (if (integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1)))
>> + (minus @1 (bit_xor @0 @1))))
>>
>> place a :c on the minus of the one not matching INTEGER_CSTs.
>
>
> Will that really work?
Yes. The :c will create a 2nd pattern with the ops of the minus swapped, thus
/* Fold (A & ~B) - (A & B) into (A ^ B) - B. */
(simplify
(minus:c (bit_and:s @0 (bit_not @1)) (bit_and:s @0 @1))
(minus (bit_xor @0 @1) @1))
(simplify
(minus (bit_and:s @0 INTEGER_CST@2) (bit_and:s @0 INTEGER_CST@1))
(if (integer_all_onesp (const_binop (BIT_XOR_EXPR, type, @2, @1)))
(minus (bit_xor @0 @1) @1)))
will implicitely add
(simplify
(minus (bit_and:s @0 @1) (bit_and:s @0 (bit_not @1))
(minus (bit_xor @0 @1) @1))
hmm, but that isn't correct for @1 == 0? So the fold-const.c code is incorrect.
Note it doesn't trigger for
int foo (int a, int b)
{
return (a & ~b) - (a & b);
}
because canonicalization makes it appear as (~b & a) - (a & b) and the
fold-const.c
fails to have a :c on the bit_and with the bit_not (you too).
Note how the fold-const.c code doesn't care about where the ~ is placed (and for
constants there isn't any explicit ~ anyway).
The code triggers on
int foo (int a, int b)
{
return ((a+1) & ~b) - ((a+1) & b);
}
int bar (int a, int b)
{
return ((a+1) & b) - ((a+1) & ~b);
}
and correctly it seems. The key is that it explicitely uses bit_not
of the first
ops b or ~b while the pattern tries to be clever and re-uses the present
bit_not - _this_ doesn't play well with using :c.
Still the duplicate INTEGER_CST pattern is not necessary.
So I suggest to modify your patch to do
+/* Fold (A & ~B) - (A & B) into (A ^ B) - B. */
+(simplify
+ (minus (bit_and:cs @0 (bit_not @1)) (bit_and:s @0 @1))
+ (minus (bit_xor @0 @1) @1))
+(simplify
+ (minus (bit_and:s @0 INTEGER_CST@2) (bit_and:s @0 INTEGER_CST@1))
+ (if (wi::bit_not (@2) == @1)
+ (minus (bit_xor @0 @1) @1)))
+
+/* Fold (A & B) - (A & ~B) into B - (A ^ B). */
+(simplify
+ (minus (bit_and:s @0 @1) (bit_and:cs @0 (bit_not @1)))
+ (minus @1 (bit_xor @0 @1)))
Richard.
>
> --
> Marc Glisse
next prev parent reply other threads:[~2015-10-21 9:49 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-07 9:54 Hurugalawadi, Naveen
2015-10-08 13:39 ` Richard Biener
2015-10-12 10:22 ` Hurugalawadi, Naveen
2015-10-12 12:49 ` Marc Glisse
2015-10-12 13:11 ` Richard Biener
2015-10-13 10:52 ` Hurugalawadi, Naveen
2015-10-13 11:38 ` Marc Glisse
2015-10-13 11:57 ` Richard Biener
2015-10-13 12:18 ` Marc Glisse
2015-10-13 12:50 ` Richard Biener
2015-10-14 5:13 ` Hurugalawadi, Naveen
2015-10-14 5:40 ` Marc Glisse
2015-10-14 10:09 ` Richard Biener
2015-10-14 10:45 ` Marc Glisse
2015-10-14 10:53 ` Richard Biener
2015-10-14 11:38 ` Marc Glisse
2015-10-15 6:11 ` Hurugalawadi, Naveen
2015-10-15 12:38 ` Richard Biener
2015-10-16 10:30 ` Hurugalawadi, Naveen
2015-10-16 11:05 ` Marc Glisse
2015-10-19 11:14 ` Hurugalawadi, Naveen
2015-10-19 13:04 ` Richard Biener
2015-10-19 11:22 ` Hurugalawadi, Naveen
2015-10-19 11:42 ` Marc Glisse
2015-10-20 6:48 ` Hurugalawadi, Naveen
2015-10-20 12:13 ` Richard Biener
2015-10-21 4:05 ` Hurugalawadi, Naveen
2015-10-21 7:26 ` Marc Glisse
2015-10-21 9:49 ` Richard Biener [this message]
2015-10-23 5:11 ` Hurugalawadi, Naveen
2015-10-23 9:07 ` Richard Biener
2015-10-24 21:37 ` Marc Glisse
2015-10-26 9:29 ` Richard Biener
2015-10-26 9:33 ` Richard Biener
2015-10-08 16:58 ` Bernd Schmidt
2015-10-08 18:03 ` Joseph Myers
2015-10-08 18:15 ` Bernd Schmidt
2015-10-09 9:32 ` 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='CAFiYyc2XZfLKBRJj1ynpRgVpiWj6fMhp9s1YRJAASSaYe=-GYA@mail.gmail.com' \
--to=richard.guenther@gmail.com \
--cc=Naveen.Hurugalawadi@caviumnetworks.com \
--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).