| Message ID | 000101cffaa3$9be23460$d3a69d20$@arm.com |
|---|---|
| State | New |
| Headers | show |
On 11/07/14 08:58, Thomas Preud'homme wrote: > Trunk revision 216971 introduced LROTATE_EXPR as the canonical representation for a byte swap of a 2 bytes value, as per [1]. However, backend expects bswaphi patterns for such operation as these operation are more specific than a rotation. This led to a number of testcases starting to fail such as gcc.target/arm/builtin-bswap16-1.c and gcc.target/aarch64/builtin-bswap-2.c (these were skipped with my configuration). This patch adds a check in expmed to expand such LROTATE_EXPR into bswaphi pattern. > > [1] https://gcc.gnu.org/ml/gcc-patches/2014-10/msg00616.html > > Note that this is unrelated to PR63761 (but I have diagnosed the root cause). > > ChangeLog entry is as follows: > > 2014-11-03 Thomas Preud'homme <thomas.preudhomme@arm.com> > > * expmed.c (expand_shift_1): Expand 8 bit rotate of 16 bit value to > bswaphi if available. Why restrict this to 8 bit rotate of a 16 bit value? Shouldn't it apply to a 16 bit rotate of a 32 bit value, or 32 bit rotate of 64 bit value? Jeff
On Fri, Nov 07, 2014 at 12:54:44PM -0700, Jeff Law wrote: > On 11/07/14 08:58, Thomas Preud'homme wrote: > >Trunk revision 216971 introduced LROTATE_EXPR as the canonical representation for a byte swap of a 2 bytes value, as per [1]. However, backend expects bswaphi patterns for such operation as these operation are more specific than a rotation. This led to a number of testcases starting to fail such as gcc.target/arm/builtin-bswap16-1.c and gcc.target/aarch64/builtin-bswap-2.c (these were skipped with my configuration). This patch adds a check in expmed to expand such LROTATE_EXPR into bswaphi pattern. > > > >[1] https://gcc.gnu.org/ml/gcc-patches/2014-10/msg00616.html > > > >Note that this is unrelated to PR63761 (but I have diagnosed the root cause). > > > >ChangeLog entry is as follows: > > > >2014-11-03 Thomas Preud'homme <thomas.preudhomme@arm.com> > > > > * expmed.c (expand_shift_1): Expand 8 bit rotate of 16 bit value to > > bswaphi if available. > Why restrict this to 8 bit rotate of a 16 bit value? Shouldn't it apply to > a 16 bit rotate of a 32 bit value, or 32 bit rotate of 64 bit value? That isn't a byteswap, but halfword swap or wordswap. 32 bit byteswap reverses 0x01020304 byte ordering into 0x04030201, while rotate 16 is 0x03040102. Jakub
> From: Jakub Jelinek [mailto:jakub@redhat.com] > Sent: Friday, November 07, 2014 8:01 PM > > Why restrict this to 8 bit rotate of a 16 bit value? Shouldn't it apply to > > a 16 bit rotate of a 32 bit value, or 32 bit rotate of 64 bit value? > > That isn't a byteswap, but halfword swap or wordswap. > 32 bit byteswap reverses 0x01020304 byte ordering into 0x04030201, > while rotate > 16 is 0x03040102. If this patch gets approved as is I'll add a comment to explain this as this is the third time someone ask me this. Best regards, Thomas
On 11/07/14 13:11, Thomas Preud'homme wrote: >> From: Jakub Jelinek [mailto:jakub@redhat.com] >> Sent: Friday, November 07, 2014 8:01 PM >>> Why restrict this to 8 bit rotate of a 16 bit value? Shouldn't it apply to >>> a 16 bit rotate of a 32 bit value, or 32 bit rotate of 64 bit value? >> >> That isn't a byteswap, but halfword swap or wordswap. >> 32 bit byteswap reverses 0x01020304 byte ordering into 0x04030201, >> while rotate >> 16 is 0x03040102. > > If this patch gets approved as is I'll add a comment to explain this as this is > the third time someone ask me this. Then I don't feel like such a bloody idiot after reading Jakub's reply :-) Jeff
On 11/07/14 08:58, Thomas Preud'homme wrote: > Trunk revision 216971 introduced LROTATE_EXPR as the canonical representation for a byte swap of a 2 bytes value, as per [1]. However, backend expects bswaphi patterns for such operation as these operation are more specific than a rotation. This led to a number of testcases starting to fail such as gcc.target/arm/builtin-bswap16-1.c and gcc.target/aarch64/builtin-bswap-2.c (these were skipped with my configuration). This patch adds a check in expmed to expand such LROTATE_EXPR into bswaphi pattern. > > [1] https://gcc.gnu.org/ml/gcc-patches/2014-10/msg00616.html > > Note that this is unrelated to PR63761 (but I have diagnosed the root cause). > > ChangeLog entry is as follows: > > 2014-11-03 Thomas Preud'homme <thomas.preudhomme@arm.com> > > * expmed.c (expand_shift_1): Expand 8 bit rotate of 16 bit value to > bswaphi if available. Approved. Sorry for the noise WRT wider modes. Jeff
> From: Jeff Law [mailto:law@redhat.com] > Sent: Friday, November 07, 2014 8:48 PM > > > > ChangeLog entry is as follows: > > > > 2014-11-03 Thomas Preud'homme <thomas.preudhomme@arm.com> > > > > * expmed.c (expand_shift_1): Expand 8 bit rotate of 16 bit value to > > bswaphi if available. > Approved. Sorry for the noise WRT wider modes. Not at all, it's a valuable feedback about the need for a comment. Especially since I was slow to realize this. Best regards, Thomas
diff --git a/gcc/expmed.c b/gcc/expmed.c index af14b99..7e86b59 100644 --- a/gcc/expmed.c +++ b/gcc/expmed.c @@ -2164,6 +2164,14 @@ expand_shift_1 (enum tree_code code, machine_mode mode, rtx shifted, code = left ? LROTATE_EXPR : RROTATE_EXPR; } + if (rotate + && CONST_INT_P (op1) + && INTVAL (op1) == BITS_PER_UNIT + && GET_MODE_SIZE (scalar_mode) == 2 + && optab_handler (bswap_optab, HImode) != CODE_FOR_nothing) + return expand_unop (HImode, bswap_optab, shifted, NULL_RTX, + unsignedp); + if (op1 == const0_rtx) return shifted;
Trunk revision 216971 introduced LROTATE_EXPR as the canonical representation for a byte swap of a 2 bytes value, as per [1]. However, backend expects bswaphi patterns for such operation as these operation are more specific than a rotation. This led to a number of testcases starting to fail such as gcc.target/arm/builtin-bswap16-1.c and gcc.target/aarch64/builtin-bswap-2.c (these were skipped with my configuration). This patch adds a check in expmed to expand such LROTATE_EXPR into bswaphi pattern. [1] https://gcc.gnu.org/ml/gcc-patches/2014-10/msg00616.html Note that this is unrelated to PR63761 (but I have diagnosed the root cause). ChangeLog entry is as follows: 2014-11-03 Thomas Preud'homme <thomas.preudhomme@arm.com> * expmed.c (expand_shift_1): Expand 8 bit rotate of 16 bit value to bswaphi if available. Is this ok for trunk? With the right configuration I could reproduce the test failure and with this patch both tests fail. Best regards, Thomas