ARM: Introduce conservative load/store optimization mode

[MatzeB]

Summary

Most of the time ARM has the CCR.UNALIGN_TRP bit set to false which
means that unaligned loads/stores do not trap and even extensive testing
will not catch these bugs. However the multi/double variants are not
affected by this bit and will still trap. In effect a more aggressive
load/store optimization will break existing (bad) code.

These bugs do not necessarily manifest in the broken code where the
misaligned pointer is formed but often later in perfectly legal code
where it is accessed. This means recompiling system libraries (which
have no alignment bugs) with a newer compiler will break existing
applications (with alignment bugs) that worked before.

So (under protest) I implemented this safe mode which limits the
formation of multi/double operations to cases that are not affected by
user code (stack operations like spills/reloads) or cases where the
normal operations trap anyway (floating point load/stores). It is
disabled by default.

[MatzeB]

Slightly improved comments.

[rengolin]

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

67

Maybe a name that has more to do with alignment?

Like "arm-unaligned-load-store", enabled by default?

test/CodeGen/ARM/ldrd.ll

19

The preferred way is to omit the check-prefix and use CHECK for "ALL".

[jmolloy]

This is a bit grim, but it looks generally OK. I'd prefer the name to be something more explanatory, like "-arm-cripple-load-store-optimizer" but I can see why you might not want to call it that.

James

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

929

Should this return "true" if unaligned accesses are disabled?

[MatzeB]

Rename switch to -arm-assume-misaligned-load-store, incorporate feedback.

[MatzeB]

Thanks for the views.

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

67

What about arm-assume-misaligned-load-store?
I'd not enable this by default, in the end there is code with undefined behavior involved for this option to be necessary, such workarounds should not be defaults.

929

I check at the place where I used the function. I'd like to think of this function as a check for a property on the program. A property of the program should be independent of the state of some cl::opt switch to I rather check that switch where the function is used.

[MatzeB]

ping, is this ok to commit Renato?

[rengolin]

Sorry, when James approved, I looked at the comments and they were all right, but didn't approve as well, as I thought unnecessary.

LGTM! :)

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

 STATISTIC(NumLdStMoved, "Number of load / store instructions moved");
 STATISTIC(NumLDRDFormed,"Number of ldrd created before allocation");
 STATISTIC(NumSTRDFormed,"Number of strd created before allocation");
 STATISTIC(NumLDRD2LDM,  "Number of ldrd instructions turned back into ldm");
 STATISTIC(NumSTRD2STM,  "Number of strd instructions turned back into stm");
 STATISTIC(NumLDRD2LDR,  "Number of ldrd instructions turned back into ldr's");
 STATISTIC(NumSTRD2STR,  "Number of strd instructions turned back into str's");
 
+/// This switch disables formation of double/multi instructions that could
+/// potentially lead to additional alignment because load/store double/multi
+/// trap even with CCR.UNALIGN_TRP disabled. \see isConservativelyFine()
+static cl::opt<bool>
+ConservativeARMLoadStoreOpt("arm-conservative-load-store", cl::Hidden,
+    cl::init(false), cl::desc("Be more conservative in ARM load/store opt"));
+
 namespace llvm {
 void initializeARMLoadStoreOptPass(PassRegistry &);
 }
 
 #define ARM_LOAD_STORE_OPT_NAME "ARM load / store optimization pass"
 
 namespace {
   /// Post- register allocation pass the combine load / store instructions to

 
 static bool isValidLSDoubleOffset(int Offset) {
   unsigned Value = abs(Offset);
   // t2LDRDi8/t2STRDi8 supports an 8 bit immediate which is internally
   // multiplied by 4.
   return (Value % 4) == 0 && Value < 1024;
 }
 
+/// Return true for loads/stores that can be combined to a double/multi
+/// operation without increasing the chances for traps because of unaligned
+/// pointers.
+static bool isConservativelyFine(const TargetSubtargetInfo &STI,
+                                 const MachineInstr &MI) {
+  // float loads/stores trap anyway, so we won't make things worse by forming
+  // multi/double variants.
+  unsigned Opcode = MI.getOpcode();
+  if (!isi32Load(Opcode) && !isi32Store(Opcode))
+    return true;
+
+  // Stack pointer alignment is usually out of the programmers control so we
+  // can conservatively allow SP-relative loads/stores.
+  if (getLoadStoreBaseOp(MI).getReg() == ARM::SP &&
+      STI.getFrameLowering()->getTransientStackAlignment() >= 4)
+    return true;
+  return false;
+}
+
 /// Find candidates for load/store multiple merge in list of MemOpQueueEntries.
 void ARMLoadStoreOpt::FormCandidates(const MemOpQueue &MemOps) {
   const MachineInstr *FirstMI = MemOps[0].MI;
   unsigned Opcode = FirstMI->getOpcode();
   bool isNotVFP = isi32Load(Opcode) || isi32Store(Opcode);
   unsigned Size = getLSMultipleTransferSize(FirstMI);
 
   unsigned SIndex = 0;
do {
     if (STI->isSwift() && !isNotVFP && (PRegNum % 2) == 1)
       CanMergeToLSMulti = false;
 
     // LDRD/STRD do not allow SP/PC. LDM/STM do not support it or have it
     // deprecated; LDM to PC is fine but cannot happen here.
     if (PReg == ARM::SP || PReg == ARM::PC)
       CanMergeToLSMulti = CanMergeToLSDouble = false;
 
+    // Should we be conservative?
+    if (ConservativeARMLoadStoreOpt && !isConservativelyFine(*STI, *MI))
+      CanMergeToLSMulti = CanMergeToLSDouble = false;
+
     // Merge following instructions where possible.
     for (unsigned I = SIndex+1; I < EIndex; ++I, ++Count) {
       int NewOffset = MemOps[I].Offset;
       if (NewOffset != Offset + (int)Size)
         break;
       const MachineOperand &MO = getLoadStoreRegOp(*MemOps[I].MI);
       unsigned Reg = MO.getReg();
       if (Reg == ARM::SP || Reg == ARM::PC)
namespace {
   };
   char ARMPreAllocLoadStoreOpt::ID = 0;
 }
 
 INITIALIZE_PASS(ARMPreAllocLoadStoreOpt, "arm-prera-load-store-opt",
                 ARM_PREALLOC_LOAD_STORE_OPT_NAME, false, false)
 
 bool ARMPreAllocLoadStoreOpt::runOnMachineFunction(MachineFunction &Fn) {
+  if (ConservativeARMLoadStoreOpt)
+    return false;
+
   TD = &Fn.getDataLayout();
   STI = &static_cast<const ARMSubtarget &>(Fn.getSubtarget());
   TII = STI->getInstrInfo();
   TRI = STI->getRegisterInfo();
   MRI = &Fn.getRegInfo();
   MF  = &Fn;
 
   bool Modified = false;

test/CodeGen/ARM/ldrd.ll

-; RUN: llc < %s -mtriple=thumbv7-apple-ios -mcpu=cortex-a8 -regalloc=fast -optimize-regalloc=0 -verify-machineinstrs | FileCheck %s -check-prefix=A8 -check-prefix=CHECK
-; RUN: llc < %s -mtriple=thumbv7-apple-ios -mcpu=cortex-m3 -regalloc=fast -optimize-regalloc=0 | FileCheck %s -check-prefix=M3 -check-prefix=CHECK
+; RUN: llc < %s -mtriple=thumbv7-apple-ios -mcpu=cortex-a8 -regalloc=fast -optimize-regalloc=0 -verify-machineinstrs | FileCheck %s -check-prefix=A8 -check-prefix=ALL -check-prefix=CHECK
+; RUN: llc < %s -mtriple=thumbv7-apple-ios -mcpu=cortex-m3 -regalloc=fast -optimize-regalloc=0 | FileCheck %s -check-prefix=M3 -check-prefix=ALL -check-prefix=CHECK
 ; rdar://6949835
-; RUN: llc < %s -mtriple=thumbv7-apple-ios -mcpu=cortex-a8 -regalloc=basic | FileCheck %s -check-prefix=BASIC -check-prefix=CHECK
-; RUN: llc < %s -mtriple=thumbv7-apple-ios -mcpu=cortex-a8 -regalloc=greedy | FileCheck %s -check-prefix=GREEDY -check-prefix=CHECK
-; RUN: llc < %s -mtriple=thumbv7-apple-ios -mcpu=swift | FileCheck %s -check-prefix=SWIFT -check-prefix=CHECK
+; RUN: llc < %s -mtriple=thumbv7-apple-ios -mcpu=cortex-a8 -regalloc=basic | FileCheck %s -check-prefix=BASIC -check-prefix=ALL -check-prefix=CHECK
+; RUN: llc < %s -mtriple=thumbv7-apple-ios -mcpu=cortex-a8 -regalloc=greedy | FileCheck %s -check-prefix=GREEDY -check-prefix=ALL -check-prefix=CHECK
+; RUN: llc < %s -mtriple=thumbv7-apple-ios -mcpu=swift | FileCheck %s -check-prefix=SWIFT -check-prefix=ALL -check-prefix=CHECK
+
+; RUN: llc < %s -mtriple=thumbv7-apple-ios -arm-conservative-load-store | FileCheck %s -check-prefix=ALL -check-prefix=CONSERVATIVE
 
 ; Magic ARM pair hints works best with linearscan / fast.
 
 @b = external global i64*
 
 ; We use the following two to force values into specific registers.
 declare i64* @get_ptr()
 declare void @use_i64(i64 %v)
 
 define void @test_ldrd(i64 %a) nounwind readonly {
-; CHECK-LABEL: test_ldrd:
+; ALL-LABEL: test_ldrd:
 ; CHECK: bl{{x?}} _get_ptr
 ; A8: ldrd r0, r1, [r0]
 ; Cortex-M3 errata 602117: LDRD with base in list may result in incorrect base
 ; register when interrupted or faulted.
 ; M3-NOT: ldrd r[[REGNUM:[0-9]+]], {{r[0-9]+}}, [r[[REGNUM]]]
+; CONSERVATIVE-NOT: ldrd
 ; CHECK: bl{{x?}} _use_i64
   %ptr = call i64* @get_ptr()
   %v = load i64, i64* %ptr, align 8
   call void @use_i64(i64 %v)
   ret void
 }
 
 ; rdar://10435045 mixed LDRi8/LDRi12
 ;
 ; In this case, LSR generate a sequence of LDRi8/LDRi12. We should be
 ; able to generate an LDRD pair here, but this is highly sensitive to
 ; regalloc hinting. So, this doubles as a register allocation
 ; test. RABasic currently does a better job within the inner loop
 ; because of its *lack* of hinting ability. Whereas RAGreedy keeps
 ; R0/R1/R2 live as the three arguments, forcing the LDRD's odd
 ; destination into R3. We then sensibly split LDRD again rather then
 ; evict another live range or use callee saved regs. Sorry if the test
 ; is sensitive to Regalloc changes, but it is an interesting case.
 ;
-; BASIC: @f
+; ALL-LABEL: f:
 ; BASIC: %bb
 ; BASIC: ldrd
 ; BASIC: str
-; GREEDY: @f
 ; GREEDY: %bb
 ; GREEDY: ldrd
 ; GREEDY: str
 define void @f(i32* nocapture %a, i32* nocapture %b, i32 %n) nounwind {
 entry:
   %0 = add nsw i32 %n, -1                         ; <i32> [#uses=2]
   %1 = icmp sgt i32 %0, 0                         ; <i1> [#uses=1]
   br i1 %1, label %bb, label %return

 }
 
 ; rdar://13978317
 ; Pair of loads not formed when lifetime markers are set.
 %struct.Test = type { i32, i32, i32 }
 
 @TestVar = external global %struct.Test
 
+; ALL-LABEL: Func1:
 define void @Func1() nounwind ssp {
-; CHECK: @Func1
 entry: 
 ; A8: movw [[BASE:r[0-9]+]], :lower16:{{.*}}TestVar{{.*}}
 ; A8: movt [[BASE]], :upper16:{{.*}}TestVar{{.*}}
 ; A8: ldrd [[FIELD1:r[0-9]+]], [[FIELD2:r[0-9]+]], {{\[}}[[BASE]], #4]
 ; A8-NEXT: add [[FIELD1]], [[FIELD2]]
 ; A8-NEXT: str [[FIELD1]], {{\[}}[[BASE]]{{\]}}
+; CONSERVATIVE-NOT: ldrd
   %orig_blocks = alloca [256 x i16], align 2
   %0 = bitcast [256 x i16]* %orig_blocks to i8*call void @llvm.lifetime.start(i64 512, i8* %0) nounwind
   %tmp1 = load i32, i32* getelementptr inbounds (%struct.Test, %struct.Test* @TestVar, i32 0, i32 1), align 4
   %tmp2 = load i32, i32* getelementptr inbounds (%struct.Test, %struct.Test* @TestVar, i32 0, i32 2), align 4
   %add = add nsw i32 %tmp2, %tmp1
   store i32 %add, i32* getelementptr inbounds (%struct.Test, %struct.Test* @TestVar, i32 0, i32 0), align 4
   call void @llvm.lifetime.end(i64 512, i8* %0) nounwind
   ret void
 }
 
 declare void @extfunc(i32, i32, i32, i32)
 
-; CHECK-LABEL: Func2:
+; ALL-LABEL: Func2:
+; CONSERVATIVE-NOT: ldrd
 ; A8: ldrd
-; A8: blx
+; ALL: bl{{x?}} _extfunc
 ; A8: pop
 define void @Func2(i32* %p) {
 entry:
   %addr0 = getelementptr i32, i32* %p, i32 0
   %addr1 = getelementptr i32, i32* %p, i32 1
   %v0 = load i32, i32* %addr0
   %v1 = load i32, i32* %addr1
   ; try to force %v0/%v1 into non-adjacent registers
   call void @extfunc(i32 %v0, i32 0, i32 0, i32 %v1)
   ret void
 }
 
-; CHECK-LABEL: strd_spill_ldrd_reload:
+; ALL-LABEL: strd_spill_ldrd_reload:
 ; A8: strd r1, r0, [sp, #-8]!
 ; M3: strd r1, r0, [sp, #-8]!
 ; BASIC: strd r1, r0, [sp, #-8]!
 ; GREEDY: strd r0, r1, [sp, #-8]!
+; CONSERVATIVE: strd r0, r1, [sp, #-8]!
 ; CHECK: @ InlineAsm Start
 ; CHECK: @ InlineAsm End
 ; A8: ldrd r2, r1, [sp]
 ; M3: ldrd r2, r1, [sp]
 ; BASIC: ldrd r2, r1, [sp]
 ; GREEDY: ldrd r1, r2, [sp]
-; CHECK: bl{{x?}} _extfunc
+; CONSERVATIVE: ldrd r1, r2, [sp]
+; ALL: bl{{x?}} _extfunc
 define void @strd_spill_ldrd_reload(i32 %v0, i32 %v1) {
   ; force %v0 and %v1 to be spilled
   call void asm sideeffect "", "~{r0},~{r1},~{r2},~{r3},~{r4},~{r5},~{r6},~{r7},~{r8},~{r9},~{r10},~{r11},~{r12},~{lr}"()
   ; force the reloaded %v0, %v1 into different registers
   call void @extfunc(i32 0, i32 %v0, i32 %v1, i32 7)
   ret void
 }
 
 declare void @extfunc2(i32*, i32, i32)
 
-; CHECK-LABEL: ldrd_postupdate_dec:
+; ALL-LABEL: ldrd_postupdate_dec:
 ; CHECK: ldrd r1, r2, [r0], #-8
-; CHECK-NEXT: bl{{x?}} _extfunc
+; CONSERVATIVE-NOT: ldrd
+; ALL: bl{{x?}} _extfunc
 define void @ldrd_postupdate_dec(i32* %p0) {
   %p0.1 = getelementptr i32, i32* %p0, i32 1
   %v0 = load i32, i32* %p0
   %v1 = load i32, i32* %p0.1
   %p1 = getelementptr i32, i32* %p0, i32 -2
   call void @extfunc2(i32* %p1, i32 %v0, i32 %v1)
   ret void
 }
 
-; CHECK-LABEL: ldrd_postupdate_inc:
+; ALL-LABEL: ldrd_postupdate_inc:
 ; CHECK: ldrd r1, r2, [r0], #8
-; CHECK-NEXT: bl{{x?}} _extfunc
+; CONSERVATIVE-NOT: ldrd
+; ALL: bl{{x?}} _extfunc
 define void @ldrd_postupdate_inc(i32* %p0) {
   %p0.1 = getelementptr i32, i32* %p0, i32 1
   %v0 = load i32, i32* %p0
   %v1 = load i32, i32* %p0.1
   %p1 = getelementptr i32, i32* %p0, i32 2
   call void @extfunc2(i32* %p1, i32 %v0, i32 %v1)
   ret void
 }
 
-; CHECK-LABEL: strd_postupdate_dec:
+; ALL-LABEL: strd_postupdate_dec:
 ; CHECK: strd r1, r2, [r0], #-8
-; CHECK-NEXT: bx lr
+; CONSERVATIVE-NOT: strd
+; ALL: bx lr
 define i32* @strd_postupdate_dec(i32* %p0, i32 %v0, i32 %v1) {
   %p0.1 = getelementptr i32, i32* %p0, i32 1
   store i32 %v0, i32* %p0
   store i32 %v1, i32* %p0.1
   %p1 = getelementptr i32, i32* %p0, i32 -2
   ret i32* %p1
 }
 
-; CHECK-LABEL: strd_postupdate_inc:
+; ALL-LABEL: strd_postupdate_inc:
 ; CHECK: strd r1, r2, [r0], #8
-; CHECK-NEXT: bx lr
+; CONSERVATIVE-NOT: strd
+; ALL: bx lr
 define i32* @strd_postupdate_inc(i32* %p0, i32 %v0, i32 %v1) {
   %p0.1 = getelementptr i32, i32* %p0, i32 1
   store i32 %v0, i32* %p0
   store i32 %v1, i32* %p0.1
   %p1 = getelementptr i32, i32* %p0, i32 2
   ret i32* %p1
 }
 
 declare void @llvm.lifetime.start(i64, i8* nocapture) nounwind
 declare void @llvm.lifetime.end(i64, i8* nocapture) nounwind

test/CodeGen/ARM/swift-vldm.ll

 ; RUN: llc < %s -mcpu=swift -mtriple=armv7s-apple-ios | FileCheck %s
+; RUN: llc < %s -arm-conservative-load-store -mcpu=swift -mtriple=armv7s-apple-ios | FileCheck %s
 
 ; Check that we avoid producing vldm instructions using d registers that
 ; begin in the most-significant half of a q register. These require more
 ; micro-ops on swift and so aren't worth combining.
 
 ; CHECK-LABEL: test_vldm
 ; CHECK: vldmia r{{[0-9]+}}, {d2, d3, d4}
 ; CHECK-NOT: vldmia r{{[0-9]+}}, {d1, d2, d3, d4}

test/CodeGen/Thumb2/thumb2-ldm.ll

-; RUN: llc < %s -mtriple=thumbv7-apple-ios -mattr=+thumb2 | FileCheck %s
+; RUN: llc < %s -mtriple=thumbv7-apple-ios -mattr=+thumb2 | FileCheck %s -check-prefix=ALL -check-prefix=CHECK
+; RUN: llc < %s -mtriple=thumbv7-apple-ios -mattr=+thumb2 -arm-conservative-load-store | FileCheck %s -check-prefix=ALL -check-prefix=CONSERVATIVE
 
 @X = external global [0 x i32]          ; <[0 x i32]*> [#uses=5]
 
 define i32 @t1() {
-; CHECK-LABEL: t1:
-; CHECK: push {r7, lr}
+; ALL-LABEL: t1:
+; ALL: push {r7, lr}
 ; CHECK: ldrd
-; CHECK: pop {r7, pc}
+; CONSERVATIVE-NOT: ldrd
+; CONSERVATIVE-NOT: ldm
+; ALL: pop {r7, pc}
         %tmp = load i32, i32* getelementptr ([0 x i32], [0 x i32]* @X, i32 0, i32 0)            ; <i32> [#uses=1]
         %tmp3 = load i32, i32* getelementptr ([0 x i32], [0 x i32]* @X, i32 0, i32 1)           ; <i32> [#uses=1]
         %tmp4 = call i32 @f1( i32 %tmp, i32 %tmp3 )                ; <i32> [#uses=1]
         ret i32 %tmp4
 }
 
 define i32 @t2() {
-; CHECK-LABEL: t2:
-; CHECK: push {r7, lr}
+; ALL-LABEL: t2:
+; ALL: push {r7, lr}
 ; CHECK: ldm
-; CHECK: pop {r7, pc}
+; CONSERVATIVE-NOT: ldrd
+; CONSERVATIVE-NOT: ldm
+; ALL: pop {r7, pc}
         %tmp = load i32, i32* getelementptr ([0 x i32], [0 x i32]* @X, i32 0, i32 2)            ; <i32> [#uses=1]
         %tmp3 = load i32, i32* getelementptr ([0 x i32], [0 x i32]* @X, i32 0, i32 3)           ; <i32> [#uses=1]
         %tmp5 = load i32, i32* getelementptr ([0 x i32], [0 x i32]* @X, i32 0, i32 4)           ; <i32> [#uses=1]
         %tmp6 = call i32 @f2( i32 %tmp, i32 %tmp3, i32 %tmp5 )             ; <i32> [#uses=1]
         ret i32 %tmp6
 }
 
 define i32 @t3() {
-; CHECK-LABEL: t3:
-; CHECK: push {r7, lr}
+; ALL-LABEL: t3:
+; ALL: push {r7, lr}
 ; CHECK: ldm
-; CHECK: pop {r7, pc}
+; CONSERVATIVE-NOT: ldrd
+; CONSERVATIVE-NOT: ldm
+; ALL: pop {r7, pc}
         %tmp = load i32, i32* getelementptr ([0 x i32], [0 x i32]* @X, i32 0, i32 1)            ; <i32> [#uses=1]
         %tmp3 = load i32, i32* getelementptr ([0 x i32], [0 x i32]* @X, i32 0, i32 2)           ; <i32> [#uses=1]
         %tmp5 = load i32, i32* getelementptr ([0 x i32], [0 x i32]* @X, i32 0, i32 3)           ; <i32> [#uses=1]
         %tmp6 = call i32 @f2( i32 %tmp, i32 %tmp3, i32 %tmp5 )             ; <i32> [#uses=1]
         ret i32 %tmp6
 }
 
+@g = common global i32* null
+
+define void @t4(i32 %a0, i32 %a1, i32 %a2) {
+; ALL-LABEL: t4:
+; ALL: stm.w sp, {r0, r1, r2}
+; ALL: blx _ext
+; ALL: ldm.w sp, {r0, r1, r2}
+; ALL: blx _f2
+  %arr = alloca [4 x i32], align 4
+  %p0 = getelementptr inbounds [4 x i32], [4 x i32]* %arr, i64 0, i64 0
+  %p1 = getelementptr inbounds [4 x i32], [4 x i32]* %arr, i64 0, i64 1
+  %p2 = getelementptr inbounds [4 x i32], [4 x i32]* %arr, i64 0, i64 2
+  store i32* %p0, i32** @g, align 8
+
+  store i32 %a0, i32* %p0, align 4
+  store i32 %a1, i32* %p1, align 4
+  store i32 %a2, i32* %p2, align 4
+  call void @ext()
+
+  %v0 = load i32, i32* %p0, align 4
+  %v1 = load i32, i32* %p1, align 4
+  %v2 = load i32, i32* %p2, align 4
+  call i32 @f2(i32 %v0, i32 %v1, i32 %v2)
+  ret void
+}
+
 declare i32 @f1(i32, i32)
 
 declare i32 @f2(i32, i32, i32)
+
+declare void @ext()