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).