[ARM] Record store with pre/post-indexed addressing as mayStore

[antoniofrighetto]

A miscompilation issue observed during machine sinking has been addressed with improved handling.

Fixes: #121299.

[llvmbot]

@llvm/pr-subscribers-backend-arm

Author: Antonio Frighetto (antoniofrighetto)

Changes

A miscompilation issue observed during machine sinking has been addressed with improved handling.

Fixes: #121299.

---

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

2 Files Affected:

• (modified) llvm/lib/Target/ARM/ARMInstrInfo.td (+2-1)
• (added) llvm/test/CodeGen/ARM/sink-store-pre-load-dependency.mir (+41)

diff --git a/llvm/lib/Target/ARM/ARMInstrInfo.td b/llvm/lib/Target/ARM/ARMInstrInfo.td
index c67177cd5a6fea..009b60c8400f02 100644
--- a/llvm/lib/Target/ARM/ARMInstrInfo.td
+++ b/llvm/lib/Target/ARM/ARMInstrInfo.td
@@ -3320,7 +3320,7 @@ def STRH_preidx: ARMPseudoInst<(outs GPR:$Rn_wb),
 }
 
 
-
+let mayStore = 1, hasSideEffects = 0 in {
 def STRH_PRE  : AI3ldstidx<0b1011, 0, 1, (outs GPR:$Rn_wb),
                            (ins GPR:$Rt, addrmode3_pre:$addr), IndexModePre,
                            StMiscFrm, IIC_iStore_bh_ru,
@@ -3352,6 +3352,7 @@ def STRH_POST : AI3ldstidx<0b1011, 0, 0, (outs GPR:$Rn_wb),
   let Inst{3-0}   = offset{3-0};    // imm3_0/Rm
   let DecoderMethod = "DecodeAddrMode3Instruction";
 }
+} // mayStore = 1, hasSideEffects = 0
 
 let mayStore = 1, hasSideEffects = 0, hasExtraSrcRegAllocReq = 1 in {
 def STRD_PRE : AI3ldstidx<0b1111, 0, 1, (outs GPR:$Rn_wb),
diff --git a/llvm/test/CodeGen/ARM/sink-store-pre-load-dependency.mir b/llvm/test/CodeGen/ARM/sink-store-pre-load-dependency.mir
new file mode 100644
index 00000000000000..92c983e2bfd1da
--- /dev/null
+++ b/llvm/test/CodeGen/ARM/sink-store-pre-load-dependency.mir
@@ -0,0 +1,41 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -o - %s -mtriple=armv7-- -run-pass=machine-sink | FileCheck %s
+
+name: sink-store-load-dep
+tracksRegLiveness: true
+stack:
+  - { id: 0, type: default, size: 8, alignment: 8 }
+body: |
+  bb.0:
+    ; CHECK-LABEL: name: sink-store-load-dep
+    ; CHECK:       bb.0:
+    ; CHECK:         [[LDRi12_:%[0-9]+]]:gpr = LDRi12 %stack.0, 0, 14 /* CC::al */, $noreg :: (load (s32))
+    ; CHECK-NEXT:    [[MOVi:%[0-9]+]]:gpr = MOVi 55296, 14 /* CC::al */, $noreg, $noreg
+    ; CHECK-NEXT:    [[ADDri1:%[0-9]+]]:gpr = ADDri [[LDRi12_:%[0-9]+]], 0, 14 /* CC::al */, $noreg, $noreg
+    ; CHECK-NEXT:    [[LDRH:%[0-9]+]]:gpr = LDRH killed [[ADDri1:%[0-9]+]], $noreg, 0, 14 /* CC::al */, $noreg :: (load (s16))
+    ; CHECK-NEXT:    [[MOVi1:%[0-9]+]]:gpr = MOVi 0, 14 /* CC::al */, $noreg, $noreg
+    ; CHECK-NEXT:    early-clobber %5:gpr = STRH_PRE [[MOVi:%[0-9]+]], [[LDRi12_:%[0-9]+]], [[MOVi1:%[0-9]+]], 0, 14 /* CC::al */, $noreg
+    ; CHECK-NEXT:    [[SUBri:%.*]]:gpr = SUBri killed [[LDRi12_:%[0-9]+]], 0, 14 /* CC::al */, $noreg, $noreg
+    ; CHECK:       bb.2:
+    ; CHECK-NEXT:    [[MOVi2:%[0-9]+]]:gpr = MOVi [[LDRH:%[0-9]+]], 14 /* CC::al */, $noreg, $noreg
+    %0:gpr = LDRi12 %stack.0, 0, 14, $noreg :: (load (s32))
+    %1:gpr = MOVi 55296, 14, $noreg, $noreg
+    %2:gpr = ADDri %0:gpr, 0, 14, $noreg, $noreg
+    %3:gpr = LDRH killed %2:gpr, $noreg, 0, 14, $noreg :: (load (s16))
+    %4:gpr = MOVi 0, 14, $noreg, $noreg
+    early-clobber %5:gpr = STRH_PRE %1:gpr, %0:gpr, %4:gpr, 0, 14, $noreg
+    %6:gpr = SUBri killed %0:gpr, 0, 14, $noreg, $noreg
+    CMPri %6:gpr, 0, 14, $noreg, implicit-def $cpsr
+    Bcc %bb.2, 3, $cpsr
+    B %bb.1
+
+  bb.1:
+    %8:gpr = MOVi 0, 14, $noreg, $noreg
+    $r0 = COPY %8:gpr
+    BX_RET 14, $noreg, implicit $r0
+
+  bb.2:
+    %9:gpr = MOVi %3:gpr, 14, $noreg, $noreg
+    $r0 = COPY %9:gpr
+    BX_RET 14, $noreg, implicit $r0
+...

[davemgreen]

Thanks for the fix for this. Does UnmodeledSideEffects not imply MayStore? There are a number of other store instructions (for various architectures) that have UnmodeledSideEffects but not MayStore.

It looks like there is an mca test that needs updating too.

[antoniofrighetto]

Updated, thanks! That doesn't look like to be the case for ARM, as UnmodeledSideEffects (setting hasSideEffects = 1 only) doesn't seem to imply MayStore (I'd personally set it explicitly anyways if it were the case though, for consistency).

[davemgreen]

This seems like an obvious improvement on it's own. I was expecting UnmodeledSideEffects to act as a load barrier, as in MachineInstr::isLoadFoldBarrier. You could argue there is another problem in MachineSink, but providing we reliably mark store as MayStore it should be OK. I'll look to see if there are other instructions in Arm that could do with the same.

LGTM, thanks.