[ARM] More care with Thumb1 writeback in ARMLoadStoreOptimizer

[scott-0]

Summary

I noticed this problem when staring at some debug output. I added an assert which catches it with existing tests and a test case.

[MatzeB]

The patch looks fine but really should have a testcase, maybe creating a .mir test is feasible here?

[scott-0]

Yes, '-stop-after' makes it straight-forward to create a test case. Added. Thanks for the pointer.

[MatzeB]

LGTM

test/CodeGen/ARM/thumb1-ldst-opt.ll

1 ↗

(On Diff #36100)

I slightly prefer

target triple = "thumbv6m-none-none"

instead of passing -mtriple to llc (mainly because it safes typing for people manually invoking llc on a testcase).

[scott-0]

Committed including changing from -mtriple ... to target triple = ...

Thanks for the review.

llvm/trunk/lib/Target/ARM/ARMLoadStoreOptimizer.cpp

if (Offset == 4 && haveIBAndDA) {
 
     // On Thumb1, it's not worth materializing a new base register without
     // clobbering the CPSR (i.e. not using ADDS/SUBS).
     if (!SafeToClobberCPSR)
       return nullptr;
 
     unsigned NewBase;
     if (isi32Load(Opcode)) {
-      // If it is a load, then just use one of the destination register to
-      // use as the new base.
+      // If it is a load, then just use one of the destination registers
+      // as the new base. Will no longer be writeback in Thumb1.
       NewBase = Regs[NumRegs-1].first;
+      Writeback = false;
     } else {
       // Find a free register that we can use as scratch register.
       moveLiveRegsBefore(MBB, InsertBefore);
       // The merged instruction does not exist yet but will use several Regs if
       // it is a Store.
       if (!isLoadSingle(Opcode))
         for (const std::pair<unsigned, bool> &R : Regs)
           LiveRegs.addReg(R.first);
MachineInstr *ARMLoadStoreOpt::CreateLoadStoreMulti(MachineBasicBlock &MBB,
   // It's safe to return here since the code to materialize a new base register
   // above is also conditional on SafeToClobberCPSR.
   if (isThumb1 && !SafeToClobberCPSR && Writeback && !BaseKill)
     return nullptr;
 
   MachineInstrBuilder MIB;
 
   if (Writeback) {
-    if (Opcode == ARM::tLDMIA)
+    assert(isThumb1 && "expected Writeback only inThumb1");
+    if (Opcode == ARM::tLDMIA) {
+      assert(!(ContainsReg(Regs, Base)) && "Thumb1 can't LDM ! with Base in Regs");
       // Update tLDMIA with writeback if necessary.
       Opcode = ARM::tLDMIA_UPD;
+    }
 
     MIB = BuildMI(MBB, InsertBefore, DL, TII->get(Opcode));
 
     // Thumb1: we might need to set base writeback when building the MI.
     MIB.addReg(Base, getDefRegState(true))
        .addReg(Base, getKillRegState(BaseKill));
 
     // The base isn't dead after a merged instruction with writeback.

llvm/trunk/test/CodeGen/ARM/thumb1-ldst-opt.ll

+; RUN: llc -stop-after block-placement -o /dev/null %s | FileCheck %s
+
+target triple = "thumbv6m-none-none"
+
+define i32* @foo(i32* readonly %p0) {
+entry:
+  %add.ptr = getelementptr inbounds i32, i32* %p0, i32 10
+  %arrayidx = getelementptr inbounds i32, i32* %p0, i32 13
+  %0 = load i32, i32* %arrayidx, align 4
+  %arrayidx1 = getelementptr inbounds i32, i32* %p0, i32 12
+  %1 = load i32, i32* %arrayidx1, align 4
+  %add = add nsw i32 %1, %0
+  %arrayidx2 = getelementptr inbounds i32, i32* %p0, i32 11
+  %2 = load i32, i32* %arrayidx2, align 4
+  %add3 = add nsw i32 %add, %2
+  %3 = load i32, i32* %add.ptr, align 4
+  %add5 = add nsw i32 %add3, %3
+  tail call void @g(i32 %add5)
+  ret i32* %p0
+}
+
+declare void @g(i32)
+
+; CHECK-LABEL: name: foo
+; CHECK: [[BASE:%r[0-7]]], {{.*}} tADDi8
+; CHECK-NOT: [[BASE]] = tLDMIA_UPD {{.*}} [[BASE]]
+; CHECK: tLDMIA killed [[BASE]], {{.*}} def [[BASE]]