From: "Roger Sayle" <roger@nextmovesoftware.com>
To: "'GCC Patches'" <gcc-patches@gcc.gnu.org>
Subject: [PATCH] PR tree-optimization/101403: Incorrect folding of ((T)bswap(x))>>C
Date: Sun, 11 Jul 2021 10:48:17 +0100 [thread overview]
Message-ID: <00de01d77639$e1302040$a39060c0$@nextmovesoftware.com> (raw)
[-- Attachment #1: Type: text/plain, Size: 1576 bytes --]
My sincere apologies for the breakage. My recent patch to fold
bswapN(x)>>C where the constant C was large enough that the result
only contains bits from the low byte, and can therefore avoid
the byte swap contains a minor logic error. The pattern contains
a convert? allowing an extension to occur between the bswap and
the shift. The logic is correct if there's no extension, or the
extension has the same sign as the shift, but I'd mistakenly
convinced myself that these couldn't have different signedness.
(T)bswap16(x)>>12 is (T)((unsigned char)x>>4) or (T)((signed char)x>>4).
The bug is that for zero-extensions to signed type T, we need to use
the unsigned char variant [the signedness of the byte shift is not
(always) the same as the signedness of T and the original shift].
Then because I'm now paranoid, I've also added a clause to handle
the hypothetical (but in practice impossible) sign-extension to an
unsigned type T, which can implemented as (T)(x<<8)>>12.
This patch has been tested on x86_64-pc-linux-gnu with a "make
bootstrap" and "make -k check" with no new failures, and a new
testcase to confirm it fixes the regression.
Ok for mainline?
2021-07-11 Roger Sayle <roger@nextmovesoftware.com>
gcc/ChangeLog
PR tree-optimization/101403
* gcc/match.pd ((T)bswap(X)>>C): Correctly handle cases where
signedness of the shift is not the same as the signedness of
the type extension.
gcc/testsuite/ChangeLog
PR tree-optimization/101403
* gcc.dg/pr101403.c: New test case.
Sorry again,
Roger
--
Roger Sayle
NextMove Software
Cambridge, UK
[-- Attachment #2: patchb2.txt --]
[-- Type: text/plain, Size: 2021 bytes --]
diff --git a/gcc/match.pd b/gcc/match.pd
index 30680d4..beb8d27 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -3659,19 +3659,31 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
{
unsigned HOST_WIDE_INT prec = TYPE_PRECISION (TREE_TYPE (@2));
unsigned HOST_WIDE_INT bits = tree_to_uhwi (@1);
+ /* If the bswap was extended before the original shift, this
+ byte (shift) has the sign of the extension, not the sign of
+ the original shift. */
+ tree st = TYPE_PRECISION (type) > prec ? TREE_TYPE (@2) : type;
}
- (if (bits + 8 == prec)
- (if (TYPE_UNSIGNED (type))
- (convert (convert:unsigned_char_type_node @0))
- (convert (convert:signed_char_type_node @0)))
- (if (bits < prec && bits + 8 > prec)
- (with
- {
- tree nst = build_int_cst (integer_type_node, bits & 7);
- tree bt = TYPE_UNSIGNED (type) ? unsigned_char_type_node
- : signed_char_type_node;
- }
- (convert (rshift:bt (convert:bt @0) {nst;}))))))))
+ /* Special case: logical right shift of sign-extended bswap.
+ (unsigned)(short)bswap16(x)>>12 is (unsigned)((short)x<<8)>>12. */
+ (if (TYPE_PRECISION (type) > prec
+ && !TYPE_UNSIGNED (TREE_TYPE (@2))
+ && TYPE_UNSIGNED (type)
+ && bits < prec && bits + 8 >= prec)
+ (with { tree nst = build_int_cst (integer_type_node, prec - 8); }
+ (rshift (convert (lshift:st (convert:st @0) {nst;})) @1))
+ (if (bits + 8 == prec)
+ (if (TYPE_UNSIGNED (st))
+ (convert (convert:unsigned_char_type_node @0))
+ (convert (convert:signed_char_type_node @0)))
+ (if (bits < prec && bits + 8 > prec)
+ (with
+ {
+ tree nst = build_int_cst (integer_type_node, bits & 7);
+ tree bt = TYPE_UNSIGNED (st) ? unsigned_char_type_node
+ : signed_char_type_node;
+ }
+ (convert (rshift:bt (convert:bt @0) {nst;})))))))))
/* bswap(x) & C1 can sometimes be simplified to (x >> C2) & C1. */
(simplify
(bit_and (convert? (bswap@2 @0)) INTEGER_CST@1)
[-- Attachment #3: pr101403.c --]
[-- Type: text/plain, Size: 295 bytes --]
/* { dg-do run } */
/* { dg-options "-O2" } */
unsigned int foo (unsigned int a)
{
unsigned int u;
unsigned short b = __builtin_bswap16 (a);
return b >> (u, 12);
}
int main (void)
{
unsigned int x = foo (0x80);
if (x != 0x0008)
__builtin_abort ();
return 0;
}
next reply other threads:[~2021-07-11 9:48 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-11 9:48 Roger Sayle [this message]
2021-07-12 6:35 ` Richard Biener
2021-07-12 7:24 ` Jakub Jelinek
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='00de01d77639$e1302040$a39060c0$@nextmovesoftware.com' \
--to=roger@nextmovesoftware.com \
--cc=gcc-patches@gcc.gnu.org \
/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).