[RISCV] Avoid forming Zilsd pairs with x0 for non-x0 register classes

[svs-quic]

The pre-RA Zilsd optimizer allowed a pair when both stored values came from the same virtual register if that virtual register was defined by a copy from X0. This is only valid when the virtual register class can actually contain X0.

Check the virtual register class before treating the value as x0_pair. This prevents forming an invalid paired store for register classes such as GPRNoX0.

Without this change we were hitting the following assertion in RISCVLoadStoreOptimizer:

assert(
    FirstReg != SecondReg &&
    "First register and second register is impossible to be same register");

[llvmorg-github-actions]

@llvm/pr-subscribers-backend-risc-v

Author: Sudharsan Veeravalli (svs-quic)

Changes

The pre-RA Zilsd optimizer allowed a pair when both stored values came from the same virtual register if that virtual register was defined by a copy from X0. This is only valid when the virtual register class can actually contain X0.

Check the virtual register class before treating the value as x0_pair. This prevents forming an invalid paired store for register classes such as GPRNoX0.

Without this change we were hitting the following assertion in RISCVLoadStoreOptimizer:

assert(
    FirstReg != SecondReg &&
    "First register and second register is impossible to be same register");

---

Full diff: https://github.com/llvm/llvm-project/pull/211019.diff

2 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVZilsdOptimizer.cpp (+2-1)
• (modified) llvm/test/CodeGen/RISCV/zilsd-ldst-opt-prera.mir (+42)

diff --git a/llvm/lib/Target/RISCV/RISCVZilsdOptimizer.cpp b/llvm/lib/Target/RISCV/RISCVZilsdOptimizer.cpp
index c7bcee5a37cbc..54908b31f6299 100644
--- a/llvm/lib/Target/RISCV/RISCVZilsdOptimizer.cpp
+++ b/llvm/lib/Target/RISCV/RISCVZilsdOptimizer.cpp
@@ -223,7 +223,8 @@ bool RISCVPreAllocZilsdOpt::canFormLdSdPair(MachineInstr *MI0,
   if (FirstReg == SecondReg) {
     const MachineInstr *FirstOpDefInst = MRI->getUniqueVRegDef(FirstReg);
     if (FirstOpDefInst->isCopy() &&
-        FirstOpDefInst->getOperand(1).getReg() == RISCV::X0)
+        FirstOpDefInst->getOperand(1).getReg() == RISCV::X0 &&
+        MRI->getRegClass(FirstReg)->contains(RISCV::X0))
       return true;
     return false;
   }
diff --git a/llvm/test/CodeGen/RISCV/zilsd-ldst-opt-prera.mir b/llvm/test/CodeGen/RISCV/zilsd-ldst-opt-prera.mir
index 20dc3a9c4598d..490586430e9b2 100644
--- a/llvm/test/CodeGen/RISCV/zilsd-ldst-opt-prera.mir
+++ b/llvm/test/CodeGen/RISCV/zilsd-ldst-opt-prera.mir
@@ -26,6 +26,13 @@
     ret void
   }
 
+  define void @basic_store_zero_no_combine(ptr %0, i32 %1, i32 %2) {
+    store i32 0, ptr %0, align 4
+    %4 = getelementptr inbounds i32, ptr %0, i32 1
+    store i32 0, ptr %4, align 4
+    ret void
+  }
+
   define i32 @basic_load_combine_8_byte_aligned(ptr %0) {
     %2 = load i32, ptr %0, align 8
     %3 = getelementptr inbounds i32, ptr %0, i32 1
@@ -364,6 +371,41 @@ body: |
     SW %1, %0, 4 :: (store (s32))
     PseudoRET
 
+...
+---
+# Basic case: two consecutive 32-bit store 0 that cannot be combined into SD
+name: basic_store_zero_no_combine
+alignment: 4
+tracksRegLiveness: true
+liveins:
+  - { reg: '$x10', virtual-reg: '%0' }
+body: |
+  bb.0:
+    liveins: $x10
+
+    ; CHECK-LABEL: name: basic_store_zero_no_combine
+    ; CHECK: liveins: $x10
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:gpr = COPY $x10
+    ; CHECK-NEXT: [[COPY1:%[0-9]+]]:gprnox0 = COPY $x0
+    ; CHECK-NEXT: SW [[COPY1]], [[COPY]], 0 :: (store (s32))
+    ; CHECK-NEXT: SW [[COPY1]], [[COPY]], 4 :: (store (s32))
+    ; CHECK-NEXT: PseudoRET
+    ;
+    ; CHECK-4BYTE-LABEL: name: basic_store_zero_no_combine
+    ; CHECK-4BYTE: liveins: $x10
+    ; CHECK-4BYTE-NEXT: {{  $}}
+    ; CHECK-4BYTE-NEXT: [[COPY:%[0-9]+]]:gpr = COPY $x10
+    ; CHECK-4BYTE-NEXT: [[COPY1:%[0-9]+]]:gprnox0 = COPY $x0
+    ; CHECK-4BYTE-NEXT: SW [[COPY1]], [[COPY]], 0 :: (store (s32))
+    ; CHECK-4BYTE-NEXT: SW [[COPY1]], [[COPY]], 4 :: (store (s32))
+    ; CHECK-4BYTE-NEXT: PseudoRET
+    %0:gpr = COPY $x10
+    %1:gprnox0 = COPY $x0
+    SW %1, %0, 0 :: (store (s32))
+    SW %1, %0, 4 :: (store (s32))
+    PseudoRET
+
 ...
 ---
 name: basic_load_combine_8_byte_aligned

[lenary]

LGTM. I think this is reasonable initially and for back-porting to 23.x

I was thinking about the change we talked about in the load-store optimiser but I can see it's pointless to create a _RV32_OPT instruction if we don't think it will ever be able to be removed by the load-store optimiser, so this is a better approach.

I think we might need to look closer at how constant registers are handled in the backend. We don't really want a %3:gpr = COPY $x0 restricted into a %3:gprnox0 = COPY $x0 because the former might be eliminated by copy propagation but the latter cannot. I think in the original testcase we were also seeing reuse of the destination of %3:gpr = COPY $x0 which I think is counterproductive in general (there's no point in %3 having a long live-range as it's constant, and reuse creates problems if one user of %3 restricts the regclass, as then all the uses unnecessarily become dependent on the copy, when maybe some of them could just use $x0 directly)

[lenary]

(Please wait for more reviews than just me)

[4vtomat]

LGTM~
I think it's good for fixing bug in release for now but it's worth to handle it later lol

[svs-quic]

/cherry-pick 6b408bf

[llvmbot]

/pull-request #211168