From: Jakub Jelinek <jakub@redhat.com>
To: Jeff Law <law@redhat.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] Fix up rotate expansion
Date: Sat, 11 May 2013 07:06:00 -0000 [thread overview]
Message-ID: <20130511070552.GS1377@tucnak.redhat.com> (raw)
In-Reply-To: <518D804A.9080609@redhat.com>
On Fri, May 10, 2013 at 05:18:34PM -0600, Jeff Law wrote:
> On 05/10/2013 08:53 AM, Jakub Jelinek wrote:
> >Hi!
> >
> >Our rotate expansion if rotate optab isn't present for the respective
> >mode looks unsafe for rotations by variable count if that count could
> >be 0, because then it invokes right or left shift by bitsize.
> >While on most targets the hw will probably do the right thing
> >(it is fine if x << 32 will either yield x or 0, in both cases
> >that ored together with x >> 0 aka x will still yield x), perhaps gcc
> >could try to optimize based on the fact that undefined behavior can't
> >happen, so IMHO it is better to generate a safer sequence.
> >
> >Ok for trunk?
> >
> >2013-05-10 Jakub Jelinek <jakub@redhat.com>
> >
> > * expmed.c (expand_shift_1): For rotations by const0_rtx just
> > return shifted. Use (-op1) & (prec - 1) as other_amount
> > instead of prec - op1.
> Found by inspection?
Yes.
> Presumably the rotate was synthesized by GCC
> from some other set of operations. To be optimizable, we'd have to
> prove the original sequence triggered undefined behaviour.
See http://gcc.gnu.org/PR57157 and
http://gcc.gnu.org/ml/gcc-patches/2013-05/msg00453.html
Until recently only the bit_rotate: code in fold-const.c was pattern
matching {L,R}ROTATE_EXPR and handled only the
(x << y) {+|^} (x >> (b - y))
patterns which is indeed undefined behavior for y == 0.
But since the above change trunk also handles the
(x << y) {+|^} (x >> ((-y) & (b - 1))
pattern which is valid even for y = 0, thus the above patch adjusts
what we generate. The info whether rotation count 0 was valid or not is
unfortunately lost, adding two new {L,R}ROTATE0_EXPR tree codes might be
overkill for this.
> Seems that we ought to have a testcase, even though it probably
> means scanning the tree dumps to pick up the undefined behaviour.
> Approved with a testcase.
I have added lots of testcases recently, for rotation by zero perhaps
something similar to rotate-1a.c from above can be added as rotate-2b.c
and rotate-4b.c, and test zero rotation.
Jakub
next prev parent reply other threads:[~2013-05-11 7:06 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-10 14:54 Jakub Jelinek
2013-05-10 23:19 ` Jeff Law
2013-05-11 7:06 ` Jakub Jelinek [this message]
2013-05-11 8:47 ` [PATCH] Fix up rotate expansion (take 2) Jakub Jelinek
2013-05-13 10:46 ` Richard Biener
2014-04-17 2:30 ` DJ Delorie
2014-04-18 11:21 ` Jakub Jelinek
2014-04-25 22:03 ` DJ Delorie
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=20130511070552.GS1377@tucnak.redhat.com \
--to=jakub@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=law@redhat.com \
/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).