public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug target/101403] New: [12 Regression] wrong code with __builtin_bswap16() at -O1
@ 2021-07-10 11:47 zsojka at seznam dot cz
2021-07-10 13:20 ` [Bug tree-optimization/101403] " pinskia at gcc dot gnu.org
` (8 more replies)
0 siblings, 9 replies; 10+ messages in thread
From: zsojka at seznam dot cz @ 2021-07-10 11:47 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101403
Bug ID: 101403
Summary: [12 Regression] wrong code with __builtin_bswap16() at
-O1
Product: gcc
Version: 12.0
Status: UNCONFIRMED
Keywords: wrong-code
Severity: normal
Priority: P3
Component: target
Assignee: unassigned at gcc dot gnu.org
Reporter: zsojka at seznam dot cz
Target Milestone: ---
Host: x86_64-pc-linux-gnu
Target: x86_64-pc-linux-gnu
Created attachment 51129
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51129&action=edit
reduced testcase
$ x86_64-pc-linux-gnu-gcc -O testcase.c
$ ./a.out
Aborted
$ x86_64-pc-linux-gnu-gcc -v
Using built-in specs.
COLLECT_GCC=/repo/gcc-trunk/binary-latest-amd64/bin/x86_64-pc-linux-gnu-gcc
COLLECT_LTO_WRAPPER=/repo/gcc-trunk/binary-trunk-r12-2234-20210709194853-g1798cac7a8b-checking-yes-rtl-df-extra-nobootstrap-amd64/bin/../libexec/gcc/x86_64-pc-linux-gnu/12.0.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /repo/gcc-trunk//configure --enable-languages=c,c++
--enable-valgrind-annotations --disable-nls --enable-checking=yes,rtl,df,extra
--disable-bootstrap --with-cloog --with-ppl --with-isl
--build=x86_64-pc-linux-gnu --host=x86_64-pc-linux-gnu
--target=x86_64-pc-linux-gnu --with-ld=/usr/bin/x86_64-pc-linux-gnu-ld
--with-as=/usr/bin/x86_64-pc-linux-gnu-as --disable-libstdcxx-pch
--prefix=/repo/gcc-trunk//binary-trunk-r12-2234-20210709194853-g1798cac7a8b-checking-yes-rtl-df-extra-nobootstrap-amd64
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 12.0.0 20210710 (experimental) (GCC)
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1
2021-07-10 11:47 [Bug target/101403] New: [12 Regression] wrong code with __builtin_bswap16() at -O1 zsojka at seznam dot cz
@ 2021-07-10 13:20 ` pinskia at gcc dot gnu.org
2021-07-10 14:05 ` [Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137 hjl.tools at gmail dot com
` (7 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: pinskia at gcc dot gnu.org @ 2021-07-10 13:20 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101403
Andrew Pinski <pinskia at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |roger at nextmovesoftware dot com
Target Milestone|--- |12.0
Component|target |tree-optimization
--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Most likely introduced by r12-2137.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137
2021-07-10 11:47 [Bug target/101403] New: [12 Regression] wrong code with __builtin_bswap16() at -O1 zsojka at seznam dot cz
2021-07-10 13:20 ` [Bug tree-optimization/101403] " pinskia at gcc dot gnu.org
@ 2021-07-10 14:05 ` hjl.tools at gmail dot com
2021-07-10 16:26 ` roger at nextmovesoftware dot com
` (6 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: hjl.tools at gmail dot com @ 2021-07-10 14:05 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101403
H.J. Lu <hjl.tools at gmail dot com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |hjl.tools at gmail dot com
Ever confirmed|0 |1
Status|UNCONFIRMED |NEW
Summary|[12 Regression] wrong code |[12 Regression] wrong code
|with __builtin_bswap16() at |with __builtin_bswap16() at
|-O1 |-O1 by r12-2137
Last reconfirmed| |2021-07-10
--- Comment #2 from H.J. Lu <hjl.tools at gmail dot com> ---
This is introduced by
commit 4c619132b3f14dc5e672a7f2f0e09cb784193559
Author: Roger Sayle <roger@nextmovesoftware.com>
Date: Thu Jul 8 11:46:14 2021 +0100
PR tree-optimization/40210: Fold (bswap(X)>>C1)&C2 to (X>>C3)&C2 in
match.pd
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137
2021-07-10 11:47 [Bug target/101403] New: [12 Regression] wrong code with __builtin_bswap16() at -O1 zsojka at seznam dot cz
2021-07-10 13:20 ` [Bug tree-optimization/101403] " pinskia at gcc dot gnu.org
2021-07-10 14:05 ` [Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137 hjl.tools at gmail dot com
@ 2021-07-10 16:26 ` roger at nextmovesoftware dot com
2021-07-10 17:59 ` jakub at gcc dot gnu.org
` (5 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: roger at nextmovesoftware dot com @ 2021-07-10 16:26 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101403
Roger Sayle <roger at nextmovesoftware dot com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |ASSIGNED
Assignee|unassigned at gcc dot gnu.org |roger at nextmovesoftware dot com
--- Comment #3 from Roger Sayle <roger at nextmovesoftware dot com> ---
Testing a fix. Sorry for the inconvenience. An even more reduced test case is:
unsigned int foo (unsigned int a)
{
unsigned int u;
/* b == 0x8000 */
unsigned short b = __builtin_bswap16 (a);
/* result = 0x0008 */
// This works: return b >> 12;
// This doesn't:
return b >> (u, 12);
}
int main (void)
{
unsigned int x = foo (0x80);
if (x != 0x0008)
__builtin_abort ();
return 0;
}
The change in signedness of the right shift (from logical to arithmetic)
triggers an oversight in my recent folding optimization.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137
2021-07-10 11:47 [Bug target/101403] New: [12 Regression] wrong code with __builtin_bswap16() at -O1 zsojka at seznam dot cz
` (2 preceding siblings ...)
2021-07-10 16:26 ` roger at nextmovesoftware dot com
@ 2021-07-10 17:59 ` jakub at gcc dot gnu.org
2021-07-10 19:33 ` zsojka at seznam dot cz
` (4 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: jakub at gcc dot gnu.org @ 2021-07-10 17:59 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101403
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |jakub at gcc dot gnu.org
--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Isn't the testcase UB? It reads uninitialized u (though it doesn't use the
value for anything).
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137
2021-07-10 11:47 [Bug target/101403] New: [12 Regression] wrong code with __builtin_bswap16() at -O1 zsojka at seznam dot cz
` (3 preceding siblings ...)
2021-07-10 17:59 ` jakub at gcc dot gnu.org
@ 2021-07-10 19:33 ` zsojka at seznam dot cz
2021-07-12 7:28 ` cvs-commit at gcc dot gnu.org
` (3 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: zsojka at seznam dot cz @ 2021-07-10 19:33 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101403
--- Comment #5 from Zdenek Sojka <zsojka at seznam dot cz> ---
(In reply to Jakub Jelinek from comment #4)
> Isn't the testcase UB? It reads uninitialized u (though it doesn't use the
> value for anything).
You are very right about that! The value is used, even though the use is
optimised out.
I am using -fsanitize=undefined when reducing testcases, but this one wasn't
caught. The only warning I see with -Wall -W is "warning: left-hand operand of
comma expression has no effect [-Wunused-value]". valgrind also doesn't
complain because, obviously, the uninitialised read is optimised out.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137
2021-07-10 11:47 [Bug target/101403] New: [12 Regression] wrong code with __builtin_bswap16() at -O1 zsojka at seznam dot cz
` (4 preceding siblings ...)
2021-07-10 19:33 ` zsojka at seznam dot cz
@ 2021-07-12 7:28 ` cvs-commit at gcc dot gnu.org
2021-07-12 7:55 ` jakub at gcc dot gnu.org
` (2 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2021-07-12 7:28 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101403
--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Roger Sayle <sayle@gcc.gnu.org>:
https://gcc.gnu.org/g:5f5fbb550af7d9d6cb56ae8f607fea0eccaa9295
commit r12-2238-g5f5fbb550af7d9d6cb56ae8f607fea0eccaa9295
Author: Roger Sayle <roger@nextmovesoftware.com>
Date: Mon Jul 12 08:24:27 2021 +0100
PR tree-optimization/101403: Incorrect folding of ((T)bswap(x))>>C
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.
2021-07-12 Roger Sayle <roger@nextmovesoftware.com>
gcc/ChangeLog
PR tree-optimization/101403
* 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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137
2021-07-10 11:47 [Bug target/101403] New: [12 Regression] wrong code with __builtin_bswap16() at -O1 zsojka at seznam dot cz
` (5 preceding siblings ...)
2021-07-12 7:28 ` cvs-commit at gcc dot gnu.org
@ 2021-07-12 7:55 ` jakub at gcc dot gnu.org
2021-07-12 9:59 ` cvs-commit at gcc dot gnu.org
2021-08-07 20:28 ` roger at nextmovesoftware dot com
8 siblings, 0 replies; 10+ messages in thread
From: jakub at gcc dot gnu.org @ 2021-07-12 7:55 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101403
--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Another question is whether we shouldn't extend the short_shift stuff in
build_binary_op also to COMPOUND_EXPRs with INTEGER_CST on the ultimate rhs.
unsigned int f1 (unsigned short x) { return x >> 12; }
unsigned int f2 (unsigned short x) { static int u; return x >> (u, 12); }
The C++ FE already handles these the same, as it does:
tree const_op1 = fold_for_warn (op1);
if (TREE_CODE (const_op1) != INTEGER_CST)
const_op1 = op1;
But it won't handle
int f ();
unsigned int f3 (unsigned short x) { return x >> (f (), 12); }
either. The C FE doesn't try to simplify op1 in any way.
I guess in those cases we don't really care about any side-effects in the
expression, what we are looking for is just whether the value is known
constant.
For diagnostics, it would be nice if we handled x >> (f (), 37) like we handle
x >> 37 if it is an out of bounds shift.
For the optimization, maybe the short_shift stuff could be repeated in
match.pd.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137
2021-07-10 11:47 [Bug target/101403] New: [12 Regression] wrong code with __builtin_bswap16() at -O1 zsojka at seznam dot cz
` (6 preceding siblings ...)
2021-07-12 7:55 ` jakub at gcc dot gnu.org
@ 2021-07-12 9:59 ` cvs-commit at gcc dot gnu.org
2021-08-07 20:28 ` roger at nextmovesoftware dot com
8 siblings, 0 replies; 10+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2021-07-12 9:59 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101403
--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Roger Sayle <sayle@gcc.gnu.org>:
https://gcc.gnu.org/g:0192c3eedbc7e6fe703abd8b321f400ddb02adf7
commit r12-2242-g0192c3eedbc7e6fe703abd8b321f400ddb02adf7
Author: Roger Sayle <roger@nextmovesoftware.com>
Date: Mon Jul 12 10:59:08 2021 +0100
Tweak testcase for PR tree-optimization/101403.
Initialize unused variable u in compound expression. Committed as obvious.
2021-07-12 Roger Sayle <roger@nextmovesoftware.com>
Jakub Jelinek <jakub@redhat.com>
gcc/testsuite/ChangeLog
PR tree-optimization/101403
* gcc.dg/pr101403.c: Avoid (unimportant) uninitialized variable.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137
2021-07-10 11:47 [Bug target/101403] New: [12 Regression] wrong code with __builtin_bswap16() at -O1 zsojka at seznam dot cz
` (7 preceding siblings ...)
2021-07-12 9:59 ` cvs-commit at gcc dot gnu.org
@ 2021-08-07 20:28 ` roger at nextmovesoftware dot com
8 siblings, 0 replies; 10+ messages in thread
From: roger at nextmovesoftware dot com @ 2021-08-07 20:28 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101403
Roger Sayle <roger at nextmovesoftware dot com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|ASSIGNED |RESOLVED
Resolution|--- |FIXED
--- Comment #9 from Roger Sayle <roger at nextmovesoftware dot com> ---
Fixed on mainline.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-08-07 20:28 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-10 11:47 [Bug target/101403] New: [12 Regression] wrong code with __builtin_bswap16() at -O1 zsojka at seznam dot cz
2021-07-10 13:20 ` [Bug tree-optimization/101403] " pinskia at gcc dot gnu.org
2021-07-10 14:05 ` [Bug tree-optimization/101403] [12 Regression] wrong code with __builtin_bswap16() at -O1 by r12-2137 hjl.tools at gmail dot com
2021-07-10 16:26 ` roger at nextmovesoftware dot com
2021-07-10 17:59 ` jakub at gcc dot gnu.org
2021-07-10 19:33 ` zsojka at seznam dot cz
2021-07-12 7:28 ` cvs-commit at gcc dot gnu.org
2021-07-12 7:55 ` jakub at gcc dot gnu.org
2021-07-12 9:59 ` cvs-commit at gcc dot gnu.org
2021-08-07 20:28 ` roger at nextmovesoftware dot com
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).