public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function
@ 2023-07-24 12:29 lloyd at randombit dot net
2023-07-24 13:22 ` [Bug target/110792] " rguenth at gcc dot gnu.org
` (20 more replies)
0 siblings, 21 replies; 22+ messages in thread
From: lloyd at randombit dot net @ 2023-07-24 12:29 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
Bug ID: 110792
Summary: GCC 13 x86_32 miscompilation of Whirlpool hash
function
Product: gcc
Version: 13.1.1
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: target
Assignee: unassigned at gcc dot gnu.org
Reporter: lloyd at randombit dot net
Target Milestone: ---
Created attachment 55619
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55619&action=edit
Reproducing testcase
Attached is a reproducing testcase for an apparent miscompilation bug in GCC 13
when compiling for 32-bit x86.
Upstream issue: https://github.com/randombit/botan/issues/3637
This code is the core loop of the Whirlpool hash function.
In the attachment if `CAUSE_CODEGEN_BUG` is defined then a particular function
is defined in an anonymous namespace. When that happens, GCC apparently
generates invalid code
$ g++ -O2 -std=c++20 -m32 whirl.cpp -o whirl
$ ./whirl
19FA61D75522A466 9B44E39C1D2E1726 C530232130D407F8 9AFEE0964997F7A7
3E83BE698B288FEB CF88E3E03C4F0757 EA8964E59B63D937 08B138CC42A66EB3
# above is the correct Whirlpool hash for the empty message
$ g++ -DCAUSE_CODEGEN_BUG -O2 -std=c++20 -m32 whirl.cpp -o whirl
$ ./whirl
zsh: segmentation fault ./whirl
If compiled with Ubsan/Asan, the code is ok:
$ g++ -DCAUSE_CODEGEN_BUG -fsanitize=address -fsanitize=undefined -O2
-std=c++20 -m32 whirl.cpp -o whirl
$ ./whirl
19FA61D75522A466 9B44E39C1D2E1726 C530232130D407F8 9AFEE0964997F7A7
3E83BE698B288FEB CF88E3E03C4F0757 EA8964E59B63D937 08B138CC42A66EB3
My GCC is
$ g++ -v
Using built-in specs.
COLLECT_GCC=/bin/g++
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-pc-linux-gnu/13.1.1/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /build/gcc/src/gcc/configure
--enable-languages=ada,c,c++,d,fortran,go,lto,objc,obj-c++ --enable-bootstrap
--prefix=/usr --libdir=/usr/lib --libexecdir=/usr/lib --mandir=/usr/share/man
--infodir=/usr/share/info --with-bugurl=https://bugs.archlinux.org/
--with-build-config=bootstrap-lto --with-linker-hash-style=gnu
--with-system-zlib --enable-__cxa_atexit --enable-cet=auto
--enable-checking=release --enable-clocale=gnu --enable-default-pie
--enable-default-ssp --enable-gnu-indirect-function --enable-gnu-unique-object
--enable-libstdcxx-backtrace --enable-link-serialization=1
--enable-linker-build-id --enable-lto --enable-multilib --enable-plugin
--enable-shared --enable-threads=posix --disable-libssp --disable-libstdcxx-pch
--disable-werror
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 13.1.1 20230429 (GCC)
This is the compiler from Arch Linux which is AFAIK stock GCC. The original
report related to the GCC 13 included in Alpine Edge.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
@ 2023-07-24 13:22 ` rguenth at gcc dot gnu.org
2023-07-24 17:11 ` [Bug target/110792] [13 Regregression] " pinskia at gcc dot gnu.org
` (19 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: rguenth at gcc dot gnu.org @ 2023-07-24 13:22 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
Richard Biener <rguenth at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Last reconfirmed| |2023-07-24
Keywords| |needs-bisection
Status|UNCONFIRMED |NEW
Ever confirmed|0 |1
--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed even without -DCAUSE_CODEGEN_BUG
Dump of assembler code for function _Z5whirlyyyyyyyy:
...
0x0804857b <+139>: xor %ebx,%edx
0x0804857d <+141>: mov 0x8049604(,%ecx,8),%ecx
=> 0x08048584 <+148>: mov 0x8049600(,%ecx,8),%ebx
0x0804858b <+155>: xor %ecx,%eax
also faults with -O1 (but not -O0).
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13 Regregression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
2023-07-24 13:22 ` [Bug target/110792] " rguenth at gcc dot gnu.org
@ 2023-07-24 17:11 ` pinskia at gcc dot gnu.org
2023-07-24 17:25 ` [Bug target/110792] [13 Regression] " pinskia at gcc dot gnu.org
` (18 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-07-24 17:11 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
Andrew Pinski <pinskia at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|--- |13.2
Known to work| |12.3.0
Summary|GCC 13 x86_32 |[13 Regregression] GCC 13
|miscompilation of Whirlpool |x86_32 miscompilation of
|hash function |Whirlpool hash function
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
2023-07-24 13:22 ` [Bug target/110792] " rguenth at gcc dot gnu.org
2023-07-24 17:11 ` [Bug target/110792] [13 Regregression] " pinskia at gcc dot gnu.org
@ 2023-07-24 17:25 ` pinskia at gcc dot gnu.org
2023-07-24 17:45 ` [Bug target/110792] [13/14 " pinskia at gcc dot gnu.org
` (17 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-07-24 17:25 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
It seems like it is one of these which is causing the miscompile:
const uint64_t s0 = WHIRL_S[get_byte<0>(x0)];
const uint64_t s1 = WHIRL_S[get_byte<1>(x1)];
const uint64_t s2 = WHIRL_S[get_byte<2>(x2)];
const uint64_t s3 = WHIRL_S[get_byte<3>(x3)];
const uint64_t s4 = WHIRL_S[get_byte<4>(x4)];
const uint64_t s5 = WHIRL_S[get_byte<5>(x5)];
const uint64_t s6 = WHIRL_S[get_byte<6>(x6)];
const uint64_t s7 = WHIRL_S[get_byte<7>(x7)];
not inlining get_byte (via gnu::noinline) allows it to run without a seg fault.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13/14 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (2 preceding siblings ...)
2023-07-24 17:25 ` [Bug target/110792] [13 Regression] " pinskia at gcc dot gnu.org
@ 2023-07-24 17:45 ` pinskia at gcc dot gnu.org
2023-07-24 18:00 ` pinskia at gcc dot gnu.org
` (16 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-07-24 17:45 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 55620
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55620&action=edit
semi-reduced testcase
Here is a reduced testcase which also fails on the trunk.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13/14 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (3 preceding siblings ...)
2023-07-24 17:45 ` [Bug target/110792] [13/14 " pinskia at gcc dot gnu.org
@ 2023-07-24 18:00 ` pinskia at gcc dot gnu.org
2023-07-24 18:02 ` pinskia at gcc dot gnu.org
` (15 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-07-24 18:00 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 55621
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55621&action=edit
almost reduced all the way
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13/14 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (4 preceding siblings ...)
2023-07-24 18:00 ` pinskia at gcc dot gnu.org
@ 2023-07-24 18:02 ` pinskia at gcc dot gnu.org
2023-07-24 18:06 ` pinskia at gcc dot gnu.org
` (14 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-07-24 18:02 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
When this is split:
(insn 12 11 18 2 (set (reg:DI 0 ax [87])
(rotate:DI (mem:DI (plus:SI (mult:SI (reg:SI 0 ax [orig:89 x0 ] [89])
(const_int 8 [0x8]))
(symbol_ref:SI ("WHIRL_S") [flags 0x2] <var_decl
0x7f3d8ca03cf0 WHIRL_S>)) [1 WHIRL_S[_1]+0 S8 A64])
(const_int 32 [0x20]))) "/app/example.cpp":4:75 971
{rotl32di2_doubleword}
(nil))
It gets split incorrectly into:
(insn 21 11 22 2 (set (reg:SI 0 ax [87])
(mem:SI (plus:SI (mult:SI (reg:SI 0 ax [orig:89 x0 ] [89])
(const_int 8 [0x8]))
(const:SI (plus:SI (symbol_ref:SI ("WHIRL_S") [flags 0x2]
<var_decl 0x7fec90c18cf0 WHIRL_S>)
(const_int 4 [0x4])))) [1 WHIRL_S[_1]+4 S4 A32]))
"/app/example.cpp":4:75 91 {*movsi_internal}
(nil))
(insn 22 21 18 2 (set (reg:SI 1 dx [+4 ])
(mem:SI (plus:SI (mult:SI (reg:SI 0 ax [orig:89 x0 ] [89])
(const_int 8 [0x8]))
(symbol_ref:SI ("WHIRL_S") [flags 0x2] <var_decl
0x7fec90c18cf0 WHIRL_S>)) [1 WHIRL_S[_1]+0 S4 A64])) "/app/example.cpp":4:75 91
{*movsi_internal}
(nil))
Basically ax set before its use in the memory ...
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13/14 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (5 preceding siblings ...)
2023-07-24 18:02 ` pinskia at gcc dot gnu.org
@ 2023-07-24 18:06 ` pinskia at gcc dot gnu.org
2023-07-24 18:16 ` pinskia at gcc dot gnu.org
` (13 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-07-24 18:06 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
Andrew Pinski <pinskia at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |roger at nextmovesoftware dot com
See Also|https://gcc.gnu.org/bugzill |
|a/show_bug.cgi?id=109109 |
--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I am almost positive it started with r13-1362-g00193676a5a3e7 which forgot to
check to see the memory operand overlaps (refers to) with the destination
register.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13/14 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (6 preceding siblings ...)
2023-07-24 18:06 ` pinskia at gcc dot gnu.org
@ 2023-07-24 18:16 ` pinskia at gcc dot gnu.org
2023-07-24 18:16 ` pinskia at gcc dot gnu.org
` (12 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-07-24 18:16 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
Andrew Pinski <pinskia at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #55620|0 |1
is obsolete| |
--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 55622
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55622&action=edit
Reduced C testcase
This reduces the previous testcase and forces the overlap ...
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13/14 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (7 preceding siblings ...)
2023-07-24 18:16 ` pinskia at gcc dot gnu.org
@ 2023-07-24 18:16 ` pinskia at gcc dot gnu.org
2023-07-24 18:31 ` pinskia at gcc dot gnu.org
` (11 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-07-24 18:16 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #7)
> Created attachment 55622 [details]
> Reduced C testcase
>
> This reduces the previous testcase and forces the overlap ...
Oh and changed it to be able to compile with the C front-end and not just the
C++ front-end.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13/14 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (8 preceding siblings ...)
2023-07-24 18:16 ` pinskia at gcc dot gnu.org
@ 2023-07-24 18:31 ` pinskia at gcc dot gnu.org
2023-07-24 18:37 ` pinskia at gcc dot gnu.org
` (10 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-07-24 18:31 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 55623
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55623&action=edit
x86_64 testcase
Here is a x86_64 testcase which shows the issue is not just with i686 but also
with int128 and x86_64 too.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13/14 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (9 preceding siblings ...)
2023-07-24 18:31 ` pinskia at gcc dot gnu.org
@ 2023-07-24 18:37 ` pinskia at gcc dot gnu.org
2023-07-25 14:56 ` roger at nextmovesoftware dot com
` (9 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-07-24 18:37 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
--- Comment #10 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #9)
> Created attachment 55623 [details]
> x86_64 testcase
>
> Here is a x86_64 testcase which shows the issue is not just with i686 but
> also with int128 and x86_64 too.
Note this started with r13-1907-g525a1a73a5a563 (rather than
r13-1362-g00193676a5a3e7 ) But both patterns have the same issue and should be
both fixed rather than just one or the other.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13/14 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (10 preceding siblings ...)
2023-07-24 18:37 ` pinskia at gcc dot gnu.org
@ 2023-07-25 14:56 ` roger at nextmovesoftware dot com
2023-07-27 9:27 ` rguenth at gcc dot gnu.org
` (8 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: roger at nextmovesoftware dot com @ 2023-07-25 14:56 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
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 #11 from Roger Sayle <roger at nextmovesoftware dot com> ---
Mine. Alas the obvious fix of adding an early clobber to the rotate doubleword
from memory alternative generates some truly terrible code (spills via memory
to SSE registers!?), but I've come up with a better solution, forcing reload to
read the operand from memory before the rotate, which appears to fix the
problem without the adverse performance impact.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13/14 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (11 preceding siblings ...)
2023-07-25 14:56 ` roger at nextmovesoftware dot com
@ 2023-07-27 9:27 ` rguenth at gcc dot gnu.org
2023-08-03 6:13 ` cvs-commit at gcc dot gnu.org
` (7 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: rguenth at gcc dot gnu.org @ 2023-07-27 9:27 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
Richard Biener <rguenth at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|13.2 |13.3
--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 13.2 is being released, retargeting bugs to GCC 13.3.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13/14 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (12 preceding siblings ...)
2023-07-27 9:27 ` rguenth at gcc dot gnu.org
@ 2023-08-03 6:13 ` cvs-commit at gcc dot gnu.org
2023-08-06 22:24 ` cvs-commit at gcc dot gnu.org
` (6 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2023-08-03 6:13 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
--- Comment #13 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:790c1f60a5662b16eb19eb4b81922995863c7571
commit r14-2939-g790c1f60a5662b16eb19eb4b81922995863c7571
Author: Roger Sayle <roger@nextmovesoftware.com>
Date: Thu Aug 3 07:12:04 2023 +0100
PR target/110792: Early clobber issues with rot32di2_doubleword on i386.
This patch is a conservative fix for PR target/110792, a wrong-code
regression affecting doubleword rotations by BITS_PER_WORD, which
effectively swaps the highpart and lowpart words, when the source to be
rotated resides in memory. The issue is that if the register used to
hold the lowpart of the destination is mentioned in the address of
the memory operand, the current define_insn_and_split unintentionally
clobbers it before reading the highpart.
Hence, for the testcase, the incorrectly generated code looks like:
salq $4, %rdi // calculate address
movq WHIRL_S+8(%rdi), %rdi // accidentally clobber addr
movq WHIRL_S(%rdi), %rbp // load (wrong) lowpart
Traditionally, the textbook way to fix this would be to add an
explicit early clobber to the instruction's constraints.
(define_insn_and_split "<insn>32di2_doubleword"
- [(set (match_operand:DI 0 "register_operand" "=r,r,r")
+ [(set (match_operand:DI 0 "register_operand" "=r,r,&r")
(any_rotate:DI (match_operand:DI 1 "nonimmediate_operand" "0,r,o")
(const_int 32)))]
but unfortunately this currently generates significantly worse code,
due to a strange choice of reloads (effectively memcpy), which ends up
looking like:
salq $4, %rdi // calculate address
movdqa WHIRL_S(%rdi), %xmm0 // load the double word in SSE reg.
movaps %xmm0, -16(%rsp) // store the SSE reg back to the
stack
movq -8(%rsp), %rdi // load highpart
movq -16(%rsp), %rbp // load lowpart
Note that reload's "&" doesn't distinguish between the memory being
early clobbered, vs the registers used in an addressing mode being
early clobbered.
The fix proposed in this patch is to remove the third alternative, that
allowed offsetable memory as an operand, forcing reload to place the
operand into a register before the rotation. This results in:
salq $4, %rdi
movq WHIRL_S(%rdi), %rax
movq WHIRL_S+8(%rdi), %rdi
movq %rax, %rbp
I believe there's a more advanced solution, by swapping the order of
the loads (if first destination register is mentioned in the address),
or inserting a lea insn (if both destination registers are mentioned
in the address), but this fix is a minimal "safe" solution, that
should hopefully be suitable for backporting.
2023-08-03 Roger Sayle <roger@nextmovesoftware.com>
gcc/ChangeLog
PR target/110792
* config/i386/i386.md (<any_rotate>ti3): For rotations by 64 bits
place operand in a register before gen_<insn>64ti2_doubleword.
(<any_rotate>di3): Likewise, for rotations by 32 bits, place
operand in a register before gen_<insn>32di2_doubleword.
(<any_rotate>32di2_doubleword): Constrain operand to be in
register.
(<any_rotate>64ti2_doubleword): Likewise.
gcc/testsuite/ChangeLog
PR target/110792
* g++.target/i386/pr110792.C: New 32-bit C++ test case.
* gcc.target/i386/pr110792.c: New 64-bit C test case.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13/14 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (13 preceding siblings ...)
2023-08-03 6:13 ` cvs-commit at gcc dot gnu.org
@ 2023-08-06 22:24 ` cvs-commit at gcc dot gnu.org
2023-08-11 3:06 ` sjames at gcc dot gnu.org
` (5 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2023-08-06 22:24 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
--- Comment #14 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:529909f9e92dd3b0ed0383f45a44d2b5f8a58958
commit r14-3012-g529909f9e92dd3b0ed0383f45a44d2b5f8a58958
Author: Roger Sayle <roger@nextmovesoftware.com>
Date: Sun Aug 6 23:19:10 2023 +0100
[Committed] Avoid FAIL of gcc.target/i386/pr110792.c
My apologies (again), I managed to mess up the 64-bit version of the
test case for PR 110792. Unlike the 32-bit version, the 64-bit case
contains exactly the same load instructions, just in a different order
making the correct and incorrect behaviours impossible to distinguish
with a scan-assembler-not. Somewhere between checking that this test
failed in a clean tree without the patch, and getting the escaping
correct, I'd failed to notice that this also FAILs in the patched tree.
Doh! Instead of removing the test completely, I've left it as a
compilation test.
The original fix is tested by the 32-bit test case.
Committed to mainline as obvious. Sorry for the incovenience.
2023-08-06 Roger Sayle <roger@nextmovesoftware.com>
gcc/testsuite/ChangeLog
PR target/110792
* gcc.target/i386/pr110792.c: Remove dg-final scan-assembler-not.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13/14 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (14 preceding siblings ...)
2023-08-06 22:24 ` cvs-commit at gcc dot gnu.org
@ 2023-08-11 3:06 ` sjames at gcc dot gnu.org
2023-09-15 17:54 ` mpolacek at gcc dot gnu.org
` (4 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: sjames at gcc dot gnu.org @ 2023-08-11 3:06 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
--- Comment #15 from Sam James <sjames at gcc dot gnu.org> ---
Roger, is this one ready to backport to releases/gcc-13 so we can pick it up
easily downstream via the branch snapshots, or do you want to let it bake on
trunk a bit longer? Cheers.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13/14 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (15 preceding siblings ...)
2023-08-11 3:06 ` sjames at gcc dot gnu.org
@ 2023-09-15 17:54 ` mpolacek at gcc dot gnu.org
2023-10-30 9:00 ` sjames at gcc dot gnu.org
` (3 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: mpolacek at gcc dot gnu.org @ 2023-09-15 17:54 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
Marek Polacek <mpolacek at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |mpolacek at gcc dot gnu.org
--- Comment #16 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
(In reply to Sam James from comment #15)
> Roger, is this one ready to backport to releases/gcc-13 so we can pick it up
> easily downstream via the branch snapshots, or do you want to let it bake on
> trunk a bit longer? Cheers.
I have the same question.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13/14 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (16 preceding siblings ...)
2023-09-15 17:54 ` mpolacek at gcc dot gnu.org
@ 2023-10-30 9:00 ` sjames at gcc dot gnu.org
2024-05-08 12:45 ` [Bug target/110792] [13 " cvs-commit at gcc dot gnu.org
` (2 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: sjames at gcc dot gnu.org @ 2023-10-30 9:00 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
Sam James <sjames at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Keywords|needs-bisection |
--- Comment #17 from Sam James <sjames at gcc dot gnu.org> ---
fwiw I backported this downstream a while ago and it's been in production
since, no complaints
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (17 preceding siblings ...)
2023-10-30 9:00 ` sjames at gcc dot gnu.org
@ 2024-05-08 12:45 ` cvs-commit at gcc dot gnu.org
2024-05-08 12:45 ` cvs-commit at gcc dot gnu.org
2024-05-08 12:47 ` rguenth at gcc dot gnu.org
20 siblings, 0 replies; 22+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2024-05-08 12:45 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
--- Comment #18 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:
https://gcc.gnu.org/g:3658dafc65a2b64989a0aa3b4007356d638f1bfa
commit r13-8723-g3658dafc65a2b64989a0aa3b4007356d638f1bfa
Author: Roger Sayle <roger@nextmovesoftware.com>
Date: Thu Aug 3 07:12:04 2023 +0100
PR target/110792: Early clobber issues with rot32di2_doubleword on i386.
This patch is a conservative fix for PR target/110792, a wrong-code
regression affecting doubleword rotations by BITS_PER_WORD, which
effectively swaps the highpart and lowpart words, when the source to be
rotated resides in memory. The issue is that if the register used to
hold the lowpart of the destination is mentioned in the address of
the memory operand, the current define_insn_and_split unintentionally
clobbers it before reading the highpart.
Hence, for the testcase, the incorrectly generated code looks like:
salq $4, %rdi // calculate address
movq WHIRL_S+8(%rdi), %rdi // accidentally clobber addr
movq WHIRL_S(%rdi), %rbp // load (wrong) lowpart
Traditionally, the textbook way to fix this would be to add an
explicit early clobber to the instruction's constraints.
(define_insn_and_split "<insn>32di2_doubleword"
- [(set (match_operand:DI 0 "register_operand" "=r,r,r")
+ [(set (match_operand:DI 0 "register_operand" "=r,r,&r")
(any_rotate:DI (match_operand:DI 1 "nonimmediate_operand" "0,r,o")
(const_int 32)))]
but unfortunately this currently generates significantly worse code,
due to a strange choice of reloads (effectively memcpy), which ends up
looking like:
salq $4, %rdi // calculate address
movdqa WHIRL_S(%rdi), %xmm0 // load the double word in SSE reg.
movaps %xmm0, -16(%rsp) // store the SSE reg back to the
stack
movq -8(%rsp), %rdi // load highpart
movq -16(%rsp), %rbp // load lowpart
Note that reload's "&" doesn't distinguish between the memory being
early clobbered, vs the registers used in an addressing mode being
early clobbered.
The fix proposed in this patch is to remove the third alternative, that
allowed offsetable memory as an operand, forcing reload to place the
operand into a register before the rotation. This results in:
salq $4, %rdi
movq WHIRL_S(%rdi), %rax
movq WHIRL_S+8(%rdi), %rdi
movq %rax, %rbp
I believe there's a more advanced solution, by swapping the order of
the loads (if first destination register is mentioned in the address),
or inserting a lea insn (if both destination registers are mentioned
in the address), but this fix is a minimal "safe" solution, that
should hopefully be suitable for backporting.
2023-08-03 Roger Sayle <roger@nextmovesoftware.com>
gcc/ChangeLog
PR target/110792
* config/i386/i386.md (<any_rotate>ti3): For rotations by 64 bits
place operand in a register before gen_<insn>64ti2_doubleword.
(<any_rotate>di3): Likewise, for rotations by 32 bits, place
operand in a register before gen_<insn>32di2_doubleword.
(<any_rotate>32di2_doubleword): Constrain operand to be in
register.
(<any_rotate>64ti2_doubleword): Likewise.
gcc/testsuite/ChangeLog
PR target/110792
* g++.target/i386/pr110792.C: New 32-bit C++ test case.
* gcc.target/i386/pr110792.c: New 64-bit C test case.
(cherry picked from commit 790c1f60a5662b16eb19eb4b81922995863c7571)
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (18 preceding siblings ...)
2024-05-08 12:45 ` [Bug target/110792] [13 " cvs-commit at gcc dot gnu.org
@ 2024-05-08 12:45 ` cvs-commit at gcc dot gnu.org
2024-05-08 12:47 ` rguenth at gcc dot gnu.org
20 siblings, 0 replies; 22+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2024-05-08 12:45 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
--- Comment #19 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:
https://gcc.gnu.org/g:3367f78ff92971ac21c67c5d82df988863605f84
commit r13-8724-g3367f78ff92971ac21c67c5d82df988863605f84
Author: Roger Sayle <roger@nextmovesoftware.com>
Date: Sun Aug 6 23:19:10 2023 +0100
[Committed] Avoid FAIL of gcc.target/i386/pr110792.c
My apologies (again), I managed to mess up the 64-bit version of the
test case for PR 110792. Unlike the 32-bit version, the 64-bit case
contains exactly the same load instructions, just in a different order
making the correct and incorrect behaviours impossible to distinguish
with a scan-assembler-not. Somewhere between checking that this test
failed in a clean tree without the patch, and getting the escaping
correct, I'd failed to notice that this also FAILs in the patched tree.
Doh! Instead of removing the test completely, I've left it as a
compilation test.
The original fix is tested by the 32-bit test case.
Committed to mainline as obvious. Sorry for the incovenience.
2023-08-06 Roger Sayle <roger@nextmovesoftware.com>
gcc/testsuite/ChangeLog
PR target/110792
* gcc.target/i386/pr110792.c: Remove dg-final scan-assembler-not.
(cherry picked from commit 529909f9e92dd3b0ed0383f45a44d2b5f8a58958)
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bug target/110792] [13 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
` (19 preceding siblings ...)
2024-05-08 12:45 ` cvs-commit at gcc dot gnu.org
@ 2024-05-08 12:47 ` rguenth at gcc dot gnu.org
20 siblings, 0 replies; 22+ messages in thread
From: rguenth at gcc dot gnu.org @ 2024-05-08 12:47 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110792
Richard Biener <rguenth at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Resolution|--- |FIXED
Known to work| |13.2.1
Known to fail| |13.2.0
Status|ASSIGNED |RESOLVED
Priority|P3 |P2
--- Comment #20 from Richard Biener <rguenth at gcc dot gnu.org> ---
Should be fixed.
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2024-05-08 12:47 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-24 12:29 [Bug target/110792] New: GCC 13 x86_32 miscompilation of Whirlpool hash function lloyd at randombit dot net
2023-07-24 13:22 ` [Bug target/110792] " rguenth at gcc dot gnu.org
2023-07-24 17:11 ` [Bug target/110792] [13 Regregression] " pinskia at gcc dot gnu.org
2023-07-24 17:25 ` [Bug target/110792] [13 Regression] " pinskia at gcc dot gnu.org
2023-07-24 17:45 ` [Bug target/110792] [13/14 " pinskia at gcc dot gnu.org
2023-07-24 18:00 ` pinskia at gcc dot gnu.org
2023-07-24 18:02 ` pinskia at gcc dot gnu.org
2023-07-24 18:06 ` pinskia at gcc dot gnu.org
2023-07-24 18:16 ` pinskia at gcc dot gnu.org
2023-07-24 18:16 ` pinskia at gcc dot gnu.org
2023-07-24 18:31 ` pinskia at gcc dot gnu.org
2023-07-24 18:37 ` pinskia at gcc dot gnu.org
2023-07-25 14:56 ` roger at nextmovesoftware dot com
2023-07-27 9:27 ` rguenth at gcc dot gnu.org
2023-08-03 6:13 ` cvs-commit at gcc dot gnu.org
2023-08-06 22:24 ` cvs-commit at gcc dot gnu.org
2023-08-11 3:06 ` sjames at gcc dot gnu.org
2023-09-15 17:54 ` mpolacek at gcc dot gnu.org
2023-10-30 9:00 ` sjames at gcc dot gnu.org
2024-05-08 12:45 ` [Bug target/110792] [13 " cvs-commit at gcc dot gnu.org
2024-05-08 12:45 ` cvs-commit at gcc dot gnu.org
2024-05-08 12:47 ` rguenth at gcc dot gnu.org
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).