]> git.ipfire.org Git - thirdparty/gcc.git/commit
PR target/110792: Early clobber issues with rot32di2_doubleword on i386.
authorRoger Sayle <roger@nextmovesoftware.com>
Thu, 3 Aug 2023 06:12:04 +0000 (07:12 +0100)
committerRoger Sayle <roger@nextmovesoftware.com>
Thu, 3 Aug 2023 06:12:04 +0000 (07:12 +0100)
commit790c1f60a5662b16eb19eb4b81922995863c7571
treea15a4c10288f11040d6db213993b1acaf5255a7d
parent373600087df596b26c10d18eb0c5082c2788808b
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.
gcc/config/i386/i386.md
gcc/testsuite/g++.target/i386/pr110792.C [new file with mode: 0644]
gcc/testsuite/gcc.target/i386/pr110792.c [new file with mode: 0644]