Fix a bug in the Thumb1 ARM Load/Store optimizer which resulted in the base register reset instruction being incorrectly placed.

[mroth]

Summary

Hi Renato, all,

This patch fixes http://llvm.org/bugs/show_bug.cgi?id=19972 - a Thumb-1 specific codegen bug in the ARM Load/Store optimization pass.

Previously, the basic block was searched for future uses of the base register, and if necessary any writeback to the base register was reset using a SUB instruction (e.g. before calling a function) just before such a use. However, this step happened *before* the merged LDM/STM instruction was built. So if there was (e.g.) a function call directly after the not-yet-formed LDM/STM, the pass would first insert a SUB instruction to reset the base register, and then (at the same location, incorrectly) insert the LDM/STM itself.

Example (taken from Bugzilla):

void bar(int*, int, int);

void foo(int *A) {
  int x = A[0];
  int y = A[1];
  bar(A, x, y);
}

This currently compiles to:
$ clang -target arm -Os -mthumb -mcpu=arm926ej-s -c t.c -S -o -

@ BB#0:                                 @ %entry
        push    {r7, lr}
        add     r7, sp, #0
        subs    r0, #8
        ldm     r0!, {r1, r2}
        bl      bar
        pop     {r7, pc}

With the patch applied, it gives the much more reasonable

  @ BB#0:
	  push	{r7, lr}
	  add	r7, sp, #0
	  ldm	r0!, {r1, r2}
	  subs	r0, #8
	  bl	bar
	  pop	{r7, pc}

(Note that we do need the subs as the ldm writes back to the base register).

The patch moves the code that emits the merged instruction forward so this happens before the call that updates base register uses and potentially inserts the SUB. I've also added a test case derived from the example above to make sure that it now works properly.

Cheers
Moritz

[rengolin]

Hi Morizt,

Apart from the comment, LGTM.

--renato

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

508

This change looks odd. Why is the previous case not relevant any more?

[mroth]

In D4084#7, @rengolin wrote:

Hi Morizt,

Apart from the comment, LGTM.

--renato

Hi Renato,

this was also a bug (which resulted in this issue happening in a few extra cases as far as I can tell) - we do

Opcode = getLoadStoreMultipleOpcode(Opcode, Mode);

a few lines before this check so Opcode can never be tLDRi (it would have been changed to tLDMIA).

I don't have commit access by the way, so someone else will have to commit this on my behalf.

Cheers
Moritz

[rengolin]

Hi Moritz,

Yes, makes sense! Committed in r210542.

cheers,
--renato

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

ARMLoadStoreOpt::MergeOps(MachineBasicBlock &MBB,
   // base register writeback.
   Opcode = getLoadStoreMultipleOpcode(Opcode, Mode);
   if (!Opcode) return false;
 
   bool Writeback = isThumb1; // Thumb1 LDM/STM have base reg writeback.
 
   // Exception: If the base register is in the input reglist, Thumb1 LDM is
   // non-writeback. Check for this.
-  if (Opcode == ARM::tLDRi && isThumb1)
+  if (Opcode == ARM::tLDMIA && isThumb1)
     for (unsigned I = 0; I < NumRegs; ++I)
       if (Base == Regs[I].first) {
         Writeback = false;
         break;
       }
 
   MachineInstrBuilder MIB;
 
   if (Writeback) {
     if (Opcode == ARM::tLDMIA)
       // Update tLDMIA with writeback if necessary.
       Opcode = ARM::tLDMIA_UPD;
 
-    // The base isn't dead after a merged instruction with writeback. Update
-    // future uses of the base with the added offset (if possible), or reset
-    // the base register as necessary.
-    if (!BaseKill)
-      UpdateBaseRegUses(MBB, MBBI, dl, Base, NumRegs, Pred, PredReg);
-
     MIB = BuildMI(MBB, MBBI, 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. Update
+    // future uses of the base with the added offset (if possible), or reset
+    // the base register as necessary.
+    if (!BaseKill)
+      UpdateBaseRegUses(MBB, MBBI, dl, Base, NumRegs, Pred, PredReg);
   } else {
     // No writeback, simply build the MachineInstr.
     MIB = BuildMI(MBB, MBBI, dl, TII->get(Opcode));
     MIB.addReg(Base, getKillRegState(BaseKill));
   }
 
   MIB.addImm(Pred).addReg(PredReg);
 

test/CodeGen/Thumb/2014-06-10-thumb1-ldst-opt-bug.ll

+; RUN: llc < %s -mtriple=thumbv6m-eabi -o - | FileCheck %s
+
+define void @foo(i32* %A) #0 {
+entry:
+; CHECK-LABEL: foo:
+; CHECK: push {r7, lr}
+; CHECK: ldm [[REG0:r[0-9]]]!,
+; CHECK-NEXT: subs [[REG0]]
+; CHECK-NEXT: bl
+  %0 = load i32* %A, align 4
+  %arrayidx1 = getelementptr inbounds i32* %A, i32 1
+  %1 = load i32* %arrayidx1, align 4
+  tail call void @bar(i32* %A, i32 %0, i32 %1) #2
+  ret void
+}
+
+declare void @bar(i32*, i32, i32) #1