ARM: fix Thumb2 CodeGen for ldrex with folded frame-index

[t.p.northover]

Summary

As reported in https://llvm.org/PR38828, Thumb2 ldrex instructions accessing local variables are not being handled correctly. In release builds this results in bad code, in debug builds an assertion.

The issue is that the t2LDREX and t2STREX instructions are marked with AddrModeNone, but rewriteT2FrameIndex relies on knowing the addressing-mode so that it can fold a stack offset into the instruction. This patch adds a new addressing-mode to handle the situation and applies it to the relevant instructions.

Neither the byte/half/dword forms, nor any ARM variant is affected because they do not allow an offset to the base register.

[efriedma]

Do you need to update estimateRSStackSizeLimit?

[t.p.northover]

Bother, yes I do. That's going to be fun to write a test for so it'll probably have to wait until tomorrow.

[t.p.northover]

Added the addressing-mode to ARMFrameLowering. The only other place that uses them appears to be optional (for an efficiency gain), so I skipped that.

[efriedma]

llvm/lib/Target/ARM/ARMFrameLowering.cpp

1519

I'm not sure I understand why AddrModeT2_i12 needs special handling, but AddrModeT2_ldrex doesn't; I think they have a similar restriction that they only support positive offsets?

[t.p.northover]

llvm/lib/Target/ARM/ARMFrameLowering.cpp

1519

The rewriteT2FrameIndex code has special cases to convert between i12 and i8 addressing-modes by switching between add/sub (see Thumb2InstrInfo.cpp:576). That means that even though the notional limit for something that's i12 is 12 bits, it may end up in an instruction with an 8-bit limit -- hence the case below.

There's no such toggle for an ldrex.

[efriedma]

LGTM

llvm/lib/Target/ARM/ARMFrameLowering.cpp

1519

I guess it's fine, then.

[t.p.northover]

Thanks. Committed as r341642.

llvm/lib/Target/ARM/ARMFrameLowering.cpp

for (auto &MI : MBB) {
         // Otherwise check the addressing mode.
         switch (MI.getDesc().TSFlags & ARMII::AddrModeMask) {
         case ARMII::AddrMode3:
         case ARMII::AddrModeT2_i8:
           Limit = std::min(Limit, (1U << 8) - 1);
           break;
         case ARMII::AddrMode5:
         case ARMII::AddrModeT2_i8s4:
+        case ARMII::AddrModeT2_ldrex:
           Limit = std::min(Limit, ((1U << 8) - 1) * 4);
           break;
         case ARMII::AddrModeT2_i12:
           // i12 supports only positive offset so these will be converted to
           // i8 opcodes. See llvm::rewriteT2FrameIndex.
           if (TFI->hasFP(MF) && AFI->hasStackFrame())
             Limit = std::min(Limit, (1U << 8) - 1);
           break;

llvm/lib/Target/ARM/ARMInstrFormats.td

 def AddrModeT1_s    : AddrMode<10>;
 def AddrModeT2_i12  : AddrMode<11>;
 def AddrModeT2_i8   : AddrMode<12>;
 def AddrModeT2_so   : AddrMode<13>;
 def AddrModeT2_pc   : AddrMode<14>;
 def AddrModeT2_i8s4 : AddrMode<15>;
 def AddrMode_i12    : AddrMode<16>;
 def AddrMode5FP16   : AddrMode<17>;
+def AddrModeT2_ldrex : AddrMode<18>;
 
 // Load / store index mode.
 class IndexMode<bits<2> val> {
   bits<2> Value = val;
 }
 def IndexModeNone : IndexMode<0>;
 def IndexModePre  : IndexMode<1>;
 def IndexModePost : IndexMode<2>;

llvm/lib/Target/ARM/ARMInstrThumb2.td

def t2LDREXB : T2I_ldrex<0b0100, (outs rGPR:$Rt), (ins addr_offset_none:$addr),
                          [(set rGPR:$Rt, (ldrex_1 addr_offset_none:$addr))]>,
                Requires<[IsThumb, HasV8MBaseline]>;
 def t2LDREXH : T2I_ldrex<0b0101, (outs rGPR:$Rt), (ins addr_offset_none:$addr),
                          AddrModeNone, 4, NoItinerary,
                          "ldrexh", "\t$Rt, $addr", "",
                          [(set rGPR:$Rt, (ldrex_2 addr_offset_none:$addr))]>,
                Requires<[IsThumb, HasV8MBaseline]>;
 def t2LDREX  : Thumb2I<(outs rGPR:$Rt), (ins t2addrmode_imm0_1020s4:$addr),
-                       AddrModeNone, 4, NoItinerary,
+                       AddrModeT2_ldrex, 4, NoItinerary,
                        "ldrex", "\t$Rt, $addr", "",
                      [(set rGPR:$Rt, (ldrex_4 t2addrmode_imm0_1020s4:$addr))]>,
                Requires<[IsThumb, HasV8MBaseline]> {
   bits<4> Rt;
   bits<12> addr;
   let Inst{31-27} = 0b11101;
   let Inst{26-20} = 0b0000101;
   let Inst{19-16} = addr{11-8};
def t2STREXH : T2I_strex<0b0101, (outs rGPR:$Rd),
                          AddrModeNone, 4, NoItinerary,
                          "strexh", "\t$Rd, $Rt, $addr", "",
                          [(set rGPR:$Rd,
                                (strex_2 rGPR:$Rt, addr_offset_none:$addr))]>,
                Requires<[IsThumb, HasV8MBaseline]>;
 
 def t2STREX  : Thumb2I<(outs rGPR:$Rd), (ins rGPR:$Rt,
                              t2addrmode_imm0_1020s4:$addr),
-                  AddrModeNone, 4, NoItinerary,
+                  AddrModeT2_ldrex, 4, NoItinerary,
                   "strex", "\t$Rd, $Rt, $addr", "",
                   [(set rGPR:$Rd,
                         (strex_4 rGPR:$Rt, t2addrmode_imm0_1020s4:$addr))]>,
                Requires<[IsThumb, HasV8MBaseline]> {
   bits<4> Rd;
   bits<4> Rt;
   bits<12> addr;
   let Inst{31-27} = 0b11101;

llvm/lib/Target/ARM/MCTargetDesc/ARMBaseInfo.h

enum AddrMode {
     AddrModeT1_4    = 9,
     AddrModeT1_s    = 10, // i8 * 4 for pc and sp relative data
     AddrModeT2_i12  = 11,
     AddrModeT2_i8   = 12,
     AddrModeT2_so   = 13,
     AddrModeT2_pc   = 14, // +/- i12 for pc relative data
     AddrModeT2_i8s4 = 15, // i8 * 4
     AddrMode_i12    = 16,
-    AddrMode5FP16   = 17  // i8 * 2
+    AddrMode5FP16   = 17,  // i8 * 2
+    AddrModeT2_ldrex = 18, // i8 * 4, with unscaled offset in MCInst
   };
 
   inline static const char *AddrModeToString(AddrMode addrmode) {
     switch (addrmode) {
     case AddrModeNone:    return "AddrModeNone";
     case AddrMode1:       return "AddrMode1";
     case AddrMode2:       return "AddrMode2";
     case AddrMode3:       return "AddrMode3";
     case AddrMode4:       return "AddrMode4";
     case AddrMode5:       return "AddrMode5";
     case AddrMode5FP16:   return "AddrMode5FP16";
     case AddrMode6:       return "AddrMode6";
     case AddrModeT1_1:    return "AddrModeT1_1";
     case AddrModeT1_2:    return "AddrModeT1_2";
     case AddrModeT1_4:    return "AddrModeT1_4";
     case AddrModeT1_s:    return "AddrModeT1_s";
     case AddrModeT2_i12:  return "AddrModeT2_i12";
     case AddrModeT2_i8:   return "AddrModeT2_i8";
     case AddrModeT2_so:   return "AddrModeT2_so";
     case AddrModeT2_pc:   return "AddrModeT2_pc";
     case AddrModeT2_i8s4: return "AddrModeT2_i8s4";
     case AddrMode_i12:    return "AddrMode_i12";
+    case AddrModeT2_ldrex:return "AddrModeT2_ldrex";
     }
   }
 
   /// Target Operand Flag enum.
   enum TOF {
     //===------------------------------------------------------------------===//
     // ARM Specific MachineOperand flags.
 

llvm/lib/Target/ARM/Thumb2InstrInfo.cpp

if (AddrMode == ARMII::AddrModeT2_i8 || AddrMode == ARMII::AddrModeT2_i12) {
         isSub = true;
       }
     } else if (AddrMode == ARMII::AddrModeT2_i8s4) {
       Offset += MI.getOperand(FrameRegIdx + 1).getImm() * 4;
       NumBits = 10; // 8 bits scaled by 4
       // MCInst operand expects already scaled value.
       Scale = 1;
       assert((Offset & 3) == 0 && "Can't encode this offset!");
+    } else if (AddrMode == ARMII::AddrModeT2_ldrex) {
+      Offset += MI.getOperand(FrameRegIdx + 1).getImm() * 4;
+      NumBits = 8; // 8 bits scaled by 4
+      Scale = 4;
+      assert((Offset & 3) == 0 && "Can't encode this offset!");
     } else {
       llvm_unreachable("Unsupported addressing mode!");
     }
 
     if (NewOpc != Opcode)
       MI.setDesc(TII.get(NewOpc));
 
     MachineOperand &ImmOp = MI.getOperand(FrameRegIdx+1);

llvm/test/CodeGen/ARM/ldrex-frame-size.ll

+; RUN: llc -mtriple=thumbv7-linux-gnueabi -o - %s | FileCheck %s
+
+; This alloca is just large enough that FrameLowering decides it needs a frame
+; to guarantee access, based on the range of ldrex.
+
+; The actual alloca size is a bit of black magic, unfortunately: the real
+; maximum accessible is 1020, but FrameLowering adds 16 bytes to its estimated
+; stack size just because so the alloca is not actually the what the limit gets
+; compared to. The important point is that we don't go up to ~4096, which is the
+; default with no strange instructions.
+define void @test_large_frame() {
+; CHECK-LABEL: test_large_frame:
+; CHECK: push
+; CHECK: sub.w sp, sp, #1004
+
+  %ptr = alloca i32, i32 251
+
+  %addr = getelementptr i32, i32* %ptr, i32 1
+  call i32 @llvm.arm.ldrex.p0i32(i32* %addr)
+  ret void
+}
+
+; This alloca is just is just the other side of the limit, so no frame
+define void @test_small_frame() {
+; CHECK-LABEL: test_small_frame:
+; CHECK-NOT: push
+; CHECK: sub.w sp, sp, #1000
+
+  %ptr = alloca i32, i32 250
+
+  %addr = getelementptr i32, i32* %ptr, i32 1
+  call i32 @llvm.arm.ldrex.p0i32(i32* %addr)
+  ret void
+}
+
+declare i32 @llvm.arm.ldrex.p0i32(i32*)

llvm/test/CodeGen/ARM/ldstrex.ll

; CHECK-T2ADDRMODE: strex {{r[0-9]+}}, {{r[0-9]+}}, [r[[ADDR]]]
   call i32 @llvm.arm.strex.p0i32(i32 0, i32* %local32)
 ; CHECK-T2ADDRMODE: mov r[[ADDR:[0-9]+]], sp
 ; CHECK-T2ADDRMODE: ldrex {{r[0-9]+}}, [r[[ADDR]]]
 ; CHECK-T2ADDRMODE: strex {{r[0-9]+}}, {{r[0-9]+}}, [r[[ADDR]]]
 
   ret void
 }
 
+define void @test_excl_addrmode_folded() {
+; CHECK-LABEL: test_excl_addrmode_folded:
+  %local = alloca i8, i32 4096
+
+  %local.0 = getelementptr i8, i8* %local, i32 4
+  %local32.0 = bitcast i8* %local.0 to i32*
+  call i32 @llvm.arm.ldrex.p0i32(i32* %local32.0)
+  call i32 @llvm.arm.strex.p0i32(i32 0, i32* %local32.0)
+; CHECK-T2ADDRMODE: ldrex {{r[0-9]+}}, [sp, #4]
+; CHECK-T2ADDRMODE: strex {{r[0-9]+}}, {{r[0-9]+}}, [sp, #4]
+
+  %local.1 = getelementptr i8, i8* %local, i32 1020
+  %local32.1 = bitcast i8* %local.1 to i32*
+  call i32 @llvm.arm.ldrex.p0i32(i32* %local32.1)
+  call i32 @llvm.arm.strex.p0i32(i32 0, i32* %local32.1)
+; CHECK-T2ADDRMODE: ldrex {{r[0-9]+}}, [sp, #1020]
+; CHECK-T2ADDRMODE: strex {{r[0-9]+}}, {{r[0-9]+}}, [sp, #1020]
+
+  ret void
+}
+
+define void @test_excl_addrmode_range() {
+; CHECK-LABEL: test_excl_addrmode_range:
+  %local = alloca i8, i32 4096
+
+  %local.0 = getelementptr i8, i8* %local, i32 1024
+  %local32.0 = bitcast i8* %local.0 to i32*
+  call i32 @llvm.arm.ldrex.p0i32(i32* %local32.0)
+  call i32 @llvm.arm.strex.p0i32(i32 0, i32* %local32.0)
+; CHECK-T2ADDRMODE: mov r[[TMP:[0-9]+]], sp
+; CHECK-T2ADDRMODE: add.w r[[ADDR:[0-9]+]], r[[TMP]], #1024
+; CHECK-T2ADDRMODE: ldrex {{r[0-9]+}}, [r[[ADDR]]]
+; CHECK-T2ADDRMODE: strex {{r[0-9]+}}, {{r[0-9]+}}, [r[[ADDR]]]
+
+  ret void
+}
+
+define void @test_excl_addrmode_align() {
+; CHECK-LABEL: test_excl_addrmode_align:
+  %local = alloca i8, i32 4096
+
+  %local.0 = getelementptr i8, i8* %local, i32 2
+  %local32.0 = bitcast i8* %local.0 to i32*
+  call i32 @llvm.arm.ldrex.p0i32(i32* %local32.0)
+  call i32 @llvm.arm.strex.p0i32(i32 0, i32* %local32.0)
+; CHECK-T2ADDRMODE: mov r[[ADDR:[0-9]+]], sp
+; CHECK-T2ADDRMODE: adds r[[ADDR:[0-9]+]], #2
+; CHECK-T2ADDRMODE: ldrex {{r[0-9]+}}, [r[[ADDR]]]
+; CHECK-T2ADDRMODE: strex {{r[0-9]+}}, {{r[0-9]+}}, [r[[ADDR]]]
+
+  ret void
+}
+
+define void @test_excl_addrmode_sign() {
+; CHECK-LABEL: test_excl_addrmode_sign:
+  %local = alloca i8, i32 4096
+
+  %local.0 = getelementptr i8, i8* %local, i32 -4
+  %local32.0 = bitcast i8* %local.0 to i32*
+  call i32 @llvm.arm.ldrex.p0i32(i32* %local32.0)
+  call i32 @llvm.arm.strex.p0i32(i32 0, i32* %local32.0)
+; CHECK-T2ADDRMODE: mov r[[ADDR:[0-9]+]], sp
+; CHECK-T2ADDRMODE: subs r[[ADDR:[0-9]+]], #4
+; CHECK-T2ADDRMODE: ldrex {{r[0-9]+}}, [r[[ADDR]]]
+; CHECK-T2ADDRMODE: strex {{r[0-9]+}}, {{r[0-9]+}}, [r[[ADDR]]]
+
+  ret void
+}
+
+define void @test_excl_addrmode_combination() {
+; CHECK-LABEL: test_excl_addrmode_combination:
+  %local = alloca i8, i32 4096
+  %unused = alloca i8, i32 64
+
+  %local.0 = getelementptr i8, i8* %local, i32 4
+  %local32.0 = bitcast i8* %local.0 to i32*
+  call i32 @llvm.arm.ldrex.p0i32(i32* %local32.0)
+  call i32 @llvm.arm.strex.p0i32(i32 0, i32* %local32.0)
+; CHECK-T2ADDRMODE: ldrex {{r[0-9]+}}, [sp, #68]
+; CHECK-T2ADDRMODE: strex {{r[0-9]+}}, {{r[0-9]+}}, [sp, #68]
+
+  ret void
+}
+
+
 ; LLVM should know, even across basic blocks, that ldrex is setting the high
 ; bits of its i32 to 0. There should be no zero-extend operation.
 define zeroext i8 @test_cross_block_zext_i8(i1 %tst, i8* %addr) {
 ; CHECK: test_cross_block_zext_i8:
 ; CHECK-NOT: uxtb
 ; CHECK-NOT: and
 ; CHECK: bx lr
   %val = call i32 @llvm.arm.ldrex.p0i8(i8* %addr)
   br i1 %tst, label %end, label %mid
 mid:
   ret i8 42
 end:
   %val8 = trunc i32 %val to i8
   ret i8 %val8
 }