[ARM] Improve handling of stack accesses in Thumb-1.

[john.brawn]

Summary

Thumb-1 only allows SP-based LDR and STR to be word-sized, and SP-base LDR, STR, and ADD only allow offsets that are a multiple of 4. Make some changes to better make use of these instructions:

• Use word loads for anyext byte and halfword loads from the stack.
• Enforce 4-byte alignment on objects accessed in this way, to ensure that the offset is valid.
• Do the same for objects whose frame index is used, in order to avoid having to use more than one ADD to generate the frame index.
• Correct how many bits of offset we think AddrModeT1_s has.
• Make the load/store optimizer able to cope with AddrModeT1_s.
• Fiddle with a bunch of tests to cope with the code generation changes that the above all causes, typically where the use of SP-based addressing causes less callee-saved registers to be used and thus saved.

[jmolloy]

Hi John,

This looks reasonable to me. I'd wait for someone else like Tim to give it the go-ahead though before committing.

Before you commit, please remove braces around one-liner if-statements.

Cheers,

James

[john.brawn]

While working on improving the spill estimation I realised that it wasn't that which was causing frame offsets to be considered invalid, but instead that ARMII::AddrModeT1_s was treated as having 5 bits of offset but it actually has 8 bits of offset (5 bits is true for FP-based loads, but FP-based loads aren't used in Thumb-1). I've attached a new patch that fixes that also.

[rengolin]

Hi John,

LGTM.

cheers,
--renato

[john.brawn]

Thanks. Could you commit this for me please?

John

[rengolin]

Hi John,

I'm seeing many failures on Thumb tests, are you seeing that, too?

Failing Tests (7):

LLVM :: CodeGen/ARM/2015-01-21-thumbv4t-ldstr-opt.ll
LLVM :: CodeGen/ARM/debug-frame-vararg.ll
LLVM :: CodeGen/ARM/frame-register.ll
LLVM :: CodeGen/ARM/thumb1-varalloc.ll
LLVM :: CodeGen/ARM/thumb1_return_sequence.ll
LLVM :: CodeGen/Thumb/stm-merge.ll
LLVM :: CodeGen/Thumb/vargs.ll

ex.

llvm/test/CodeGen/Thumb/stm-merge.ll:13:10: error: expected string not found in input
; CHECK: stm r[[BASE:[0-9]]]!, {{.*}}
         ^
<stdin>:23:2: note: scanning from here
 .fnstart
 ^
<stdin>:31:2: note: possible intended match here
 str r0, [sp]
 ^

or

llvm/test/CodeGen/Thumb/vargs.ll:39:10: error: expected string not found in input
; CHECK: pop
         ^
<stdin>:42:6: note: scanning from here
 pop {r3}
     ^
<stdin>:63:50: note: possible intended match here
 .section __DATA,__nl_symbol_ptr,non_lazy_symbol_pointers
                                             ^

[john.brawn]

Yes I see them. Also, I've realised that the patch breaks things on big-endian.

John

[john.brawn]

New patch with test failures fixed. Some test failures were due to the load/store optimizer not understanding the AddrModeT1_s loads/stores, so it now does understand them, but an annoyingly large amount were due to tests needing adjustment due to different codegen. The widening of loads also now only happens little-endian, and not in thumb2 as then we have other instructions with more flexible addressing.

[rengolin]

In D7594#125493, @john.brawn wrote:

New patch with test failures fixed. Some test failures were due to the load/store optimizer not understanding the AddrModeT1_s loads/stores, so it now does understand them, but an annoyingly large amount were due to tests needing adjustment due to different codegen. The widening of loads also now only happens little-endian, and not in thumb2 as then we have other instructions with more flexible addressing.

Most of the tests were not generic enough to begin with. But due to their specific nature, I'm not sure we can improve them more than what you did.

James, are you happy with the LoadStoreOptimizer changes?

Otherwise, LGTM. Thanks!

[jmolloy]

Yeah this looks fine.

[john.brawn]

Thanks. Could you or Renato commit this for me please?

John

[rengolin]

I'm running the tests now, will commit as soon as it finishes.

cheers,
--renato

[rengolin]

r230496

lib/Target/ARM/ARMISelDAGToDAG.cpp

                                             SDValue &Base, SDValue &OffImm) {
   if (N.getOpcode() == ISD::FrameIndex) {
     int FI = cast<FrameIndexSDNode>(N)->getIndex();
+    // Only multiples of 4 are allowed for the offset, so the frame object
+    // alignment must be at least 4.
+    MachineFrameInfo *MFI = MF->getFrameInfo();
+    if (MFI->getObjectAlignment(FI) < 4) {
+      MFI->setObjectAlignment(FI, 4);
+    }
     Base = CurDAG->getTargetFrameIndex(FI, TLI->getPointerTy());
     OffImm = CurDAG->getTargetConstant(0, MVT::i32);
     return true;

       Base = N.getOperand(0);
       if (Base.getOpcode() == ISD::FrameIndex) {
         int FI = cast<FrameIndexSDNode>(Base)->getIndex();
+        // For LHS+RHS to result in an offset that's a multiple of 4 the object
+        // indexed by the LHS must be 4-byte aligned.
+        MachineFrameInfo *MFI = MF->getFrameInfo();
+        if (MFI->getObjectAlignment(FI) < 4) {
+          MFI->setObjectAlignment(FI, 4);
+        }
         Base = CurDAG->getTargetFrameIndex(FI, TLI->getPointerTy());
       }
       OffImm = CurDAG->getTargetConstant(RHSC, MVT::i32);

     int FI = cast<FrameIndexSDNode>(N)->getIndex();
     SDValue TFI = CurDAG->getTargetFrameIndex(FI, TLI->getPointerTy());
     if (Subtarget->isThumb1Only()) {
+      // Set the alignment of the frame object to 4, to avoid having to generate
+      // more than one ADD
+      MachineFrameInfo *MFI = MF->getFrameInfo();
+      if (MFI->getObjectAlignment(FI) < 4) {
+        MFI->setObjectAlignment(FI, 4);
+      }
       return CurDAG->SelectNodeTo(N, ARM::tADDframe, MVT::i32, TFI,
                                   CurDAG->getTargetConstant(0, MVT::i32));
     } else {

lib/Target/ARM/ARMInstrThumb.td

 def : T1Pat<(zextloadi1 t_addrmode_is1:$addr),
             (tLDRBi t_addrmode_is1:$addr)>;
 
+// extload from the stack -> word load from the stack, as it avoids having to
+// materialize the base in a separate register.
+def : T1Pat<(extloadi1  t_addrmode_sp:$addr), (tLDRspi t_addrmode_sp:$addr)>;
+def : T1Pat<(extloadi8  t_addrmode_sp:$addr), (tLDRspi t_addrmode_sp:$addr)>;
+def : T1Pat<(extloadi16 t_addrmode_sp:$addr), (tLDRspi t_addrmode_sp:$addr)>;
+
 // extload -> zextload
 def : T1Pat<(extloadi1  t_addrmode_rrs1:$addr), (tLDRBr t_addrmode_rrs1:$addr)>;
 def : T1Pat<(extloadi1  t_addrmode_is1:$addr),  (tLDRBi t_addrmode_is1:$addr)>;

test/CodeGen/Thumb/stack-access.ll

+; RUN: llc -mtriple=thumb-eabi < %s -o - | FileCheck %s
+
+; Check that stack addresses are generated using a single ADD
+define void @test1(i8** %p) {
+  %x = alloca i8, align 1
+  %y = alloca i8, align 1
+  %z = alloca i8, align 1
+; CHECK: add r1, sp, #8
+; CHECK: str r1, [r0]
+  store i8* %x, i8** %p, align 4
+; CHECK: add r1, sp, #4
+; CHECK: str r1, [r0]
+  store i8* %y, i8** %p, align 4
+; CHECK: mov r1, sp
+; CHECK: str r1, [r0]
+  store i8* %z, i8** %p, align 4
+  ret void
+}
+
+; Stack offsets larger than 1020 still need two ADDs
+define void @test2([1024 x i8]** %p) {
+  %arr1 = alloca [1024 x i8], align 1
+  %arr2 = alloca [1024 x i8], align 1
+; CHECK: add r1, sp, #1020
+; CHECK: adds r1, #4
+; CHECK: str r1, [r0]
+  store [1024 x i8]* %arr1, [1024 x i8]** %p, align 4
+; CHECK: mov r1, sp
+; CHECK: str r1, [r0]
+  store [1024 x i8]* %arr2, [1024 x i8]** %p, align 4
+  ret void
+}