[ARM] Expand the range of allowed post-incs in load/store optimizer

[dmgreen]

Summary

Currently the load/store optimizer will only fold in increments of the same size as the load/store. This patch expands that to any legal immediate for the post-inc instruction.

[SjoerdMeijer]

LGTM

[mstorsjo]

This causes miscompiles for me (code that gives the wrong result at runtime). I've still to narrow it down to something that can be inspected though...

[dmgreen]

OK thanks. I was hoping/expecting any problems to come up at compile-time.

Which target are you compiling for? That might at least narrow down what type of instruction it is.

[mstorsjo]

In D95885#2574321, @dmgreen wrote:

OK thanks. I was hoping/expecting any problems to come up at compile-time.

Which target are you compiling for? That might at least narrow down what type of instruction it is.

I'm building for armv7-w64-mingw32 (i.e. armv7-windows-gnu), which is implicitly thumb(2).

[mstorsjo]

Ok, I think I've got it reduced fairly well. Use https://martin.st/temp/vp8-bitstream.c as souce.

Compiled with clang -target armv7-w64-mingw32 vp8-bitstream.c -S -o bad.s -O2, I'm getting this diff:

$ diff -u good.s bad.s
@@ -3831,8 +3832,7 @@
        pop.w   {r4, r5, r6, r7, r8, r9, r10, r11, pc}
 .LBB6_246:                              @ %if.else332
        add.w   r0, r9, #110592
-       add.w   r0, r0, #1584           
-       ldrd    r1, r2, [r0]
+       ldrd    r1, r2, [r0, #1584]!    
        mov     r0, r6
        bl      vp8_pack_tokens
 .LBB6_247:                              @ %if.end335   

If I try to assemble bad.s, I'm getting this error:

bad.s:3835:15: error: invalid operand for instruction
        ldrd    r1, r2, [r0, #1584]!
                        ^

If compiling directly to an object file, no error is produced.

[dmgreen]

Thanks. That certainly sounds out of range! I'll take a look and try to add some extra testing.

llvm/lib/Target/ARM/ARMBaseInstrInfo.h

inline bool isLegalAddressImm(unsigned Opcode, int Imm,
   unsigned AddrMode = (Desc.TSFlags & ARMII::AddrModeMask);
   switch (AddrMode) {
   case ARMII::AddrModeT2_i7:
     return std::abs(Imm) < (((1 << 7) * 1) - 1);
   case ARMII::AddrModeT2_i7s2:
     return std::abs(Imm) < (((1 << 7) * 2) - 1) && Imm % 2 == 0;
   case ARMII::AddrModeT2_i7s4:
     return std::abs(Imm) < (((1 << 7) * 4) - 1) && Imm % 4 == 0;
+  case ARMII::AddrMode2:
   case ARMII::AddrModeT2_i8:
     return std::abs(Imm) < (((1 << 8) * 1) - 1);
   case ARMII::AddrModeT2_i12:
     return Imm >= 0 && Imm < (((1 << 12) * 1) - 1);
+  case ARMII::AddrModeT2_i8s4:
+    return std::abs(Imm) < (((1 << 8) * 4) - 1) && Imm % 4 == 0;
   default:
     llvm_unreachable("Unhandled Addressing mode");
   }
 }
 
 // Return true if the given intrinsic is a gather
 inline bool isGather(IntrinsicInst *IntInst) {
   if (IntInst == nullptr)

llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp

MachineBasicBlock::iterator MergeInstr
     = findIncDecBefore(MBBI, Base, Pred, PredReg, Offset);
   unsigned NewOpc;
   if (!isAM5 && Offset == Bytes) {
     NewOpc = getPreIndexedLoadStoreOpcode(Opcode, ARM_AM::add);
   } else if (Offset == -Bytes) {
     NewOpc = getPreIndexedLoadStoreOpcode(Opcode, ARM_AM::sub);
   } else {
     MergeInstr = findIncDecAfter(MBBI, Base, Pred, PredReg, Offset, TRI);
-    if (Offset == Bytes) {
-      NewOpc = getPostIndexedLoadStoreOpcode(Opcode, ARM_AM::add);
-    } else if (!isAM5 && Offset == -Bytes) {
+    if (MergeInstr == MBB.end())
+      return false;
+
+    if ((isAM5 && Offset != Bytes) ||
+        (!isAM5 && !isLegalAddressImm(NewOpc, Offset, TII))) {
       NewOpc = getPostIndexedLoadStoreOpcode(Opcode, ARM_AM::sub);
-    } else
+      if (isAM5 || !isLegalAddressImm(NewOpc, Offset, TII))
         return false;
     }
+  }
   LLVM_DEBUG(dbgs() << "  Erasing old increment: " << *MergeInstr);
   MBB.erase(MergeInstr);
 
   ARM_AM::AddrOpc AddSub = Offset < 0 ? ARM_AM::sub : ARM_AM::add;
 
   bool isLd = isLoadSingle(Opcode);
   if (isAM5) {
     // VLDM[SD]_UPD, VSTM[SD]_UPD
if (isAM2) {
                 .addReg(Base)
                 .addImm(Offset)
                 .addImm(Pred)
                 .addReg(PredReg)
                 .cloneMemRefs(*MI);
         (void)MIB;
         LLVM_DEBUG(dbgs() << "  Added new instruction: " << *MIB);
       } else {
-        int Imm = ARM_AM::getAM2Opc(AddSub, Bytes, ARM_AM::no_shift);
+        int Imm = ARM_AM::getAM2Opc(AddSub, abs(Offset), ARM_AM::no_shift);
         auto MIB =
             BuildMI(MBB, MBBI, DL, TII->get(NewOpc), MI->getOperand(0).getReg())
                 .addReg(Base, RegState::Define)
                 .addReg(Base)
                 .addReg(0)
                 .addImm(Imm)
                 .add(predOps(Pred, PredReg))
                 .cloneMemRefs(*MI);
if (isAM2) {
       LLVM_DEBUG(dbgs() << "  Added new instruction: " << *MIB);
     }
   } else {
     MachineOperand &MO = MI->getOperand(0);
     // FIXME: post-indexed stores use am2offset_imm, which still encodes
     // the vestigal zero-reg offset register. When that's fixed, this clause
     // can be removed entirely.
     if (isAM2 && NewOpc == ARM::STR_POST_IMM) {
-      int Imm = ARM_AM::getAM2Opc(AddSub, Bytes, ARM_AM::no_shift);
+      int Imm = ARM_AM::getAM2Opc(AddSub, abs(Offset), ARM_AM::no_shift);
       // STR_PRE, STR_POST
       auto MIB = BuildMI(MBB, MBBI, DL, TII->get(NewOpc), Base)
                      .addReg(MO.getReg(), getKillRegState(MO.isKill()))
                      .addReg(Base)
                      .addReg(0)
                      .addImm(Imm)
                      .add(predOps(Pred, PredReg))
                      .cloneMemRefs(*MI);
bool ARMLoadStoreOpt::MergeBaseUpdateLSDouble(MachineInstr &MI) const {
   Register PredReg;
   ARMCC::CondCodes Pred = getInstrPredicate(MI, PredReg);
   MachineBasicBlock::iterator MBBI(MI);
   MachineBasicBlock &MBB = *MI.getParent();
   int Offset;
   MachineBasicBlock::iterator MergeInstr = findIncDecBefore(MBBI, Base, Pred,
                                                             PredReg, Offset);
   unsigned NewOpc;
-  if (Offset == 8 || Offset == -8) {
+  if (Offset != 0) {
     NewOpc = Opcode == ARM::t2LDRDi8 ? ARM::t2LDRD_PRE : ARM::t2STRD_PRE;
   } else {
     MergeInstr = findIncDecAfter(MBBI, Base, Pred, PredReg, Offset, TRI);
-    if (Offset == 8 || Offset == -8) {
-      NewOpc = Opcode == ARM::t2LDRDi8 ? ARM::t2LDRD_POST : ARM::t2STRD_POST;
-    } else
+    if (MergeInstr == MBB.end())
+      return false;
+    if (!isLegalAddressImm(NewOpc, Offset, TII))
       return false;
   }
   LLVM_DEBUG(dbgs() << "  Erasing old increment: " << *MergeInstr);
   MBB.erase(MergeInstr);
 
   DebugLoc DL = MI.getDebugLoc();
   MachineInstrBuilder MIB = BuildMI(MBB, MBBI, DL, TII->get(NewOpc));
   if (NewOpc == ARM::t2LDRD_PRE || NewOpc == ARM::t2LDRD_POST) {

llvm/test/CodeGen/ARM/arm-shrink-wrapping.ll

 ; ARM-DISABLE-NEXT:    pop {r7, pc}
 ;
 ; THUMB-ENABLE-LABEL: callVariadicFunc:
 ; THUMB-ENABLE:       @ %bb.0: @ %entry
 ; THUMB-ENABLE-NEXT:    cbz r0, LBB7_2
 ; THUMB-ENABLE-NEXT:  @ %bb.1: @ %if.then
 ; THUMB-ENABLE-NEXT:    push {r7, lr}
 ; THUMB-ENABLE-NEXT:    mov r7, sp
-; THUMB-ENABLE-NEXT:    sub sp, #12
+; THUMB-ENABLE-NEXT:    strd r1, r1, [sp, #-12]!
 ; THUMB-ENABLE-NEXT:    mov r0, r1
 ; THUMB-ENABLE-NEXT:    mov r2, r1
 ; THUMB-ENABLE-NEXT:    mov r3, r1
-; THUMB-ENABLE-NEXT:    strd r1, r1, [sp]
 ; THUMB-ENABLE-NEXT:    str r1, [sp, #8]
 ; THUMB-ENABLE-NEXT:    bl _someVariadicFunc
 ; THUMB-ENABLE-NEXT:    lsls r0, r0, #3
 ; THUMB-ENABLE-NEXT:    add sp, #12
 ; THUMB-ENABLE-NEXT:    pop {r7, pc}
 ; THUMB-ENABLE-NEXT:  LBB7_2: @ %if.else
 ; THUMB-ENABLE-NEXT:    lsls r0, r1, #1
 ; THUMB-ENABLE-NEXT:    bx lr

llvm/test/CodeGen/Thumb2/mve-float32regloops.ll

 ; CHECK-NEXT:    vmov r8, s8
 ; CHECK-NEXT:    vldrw.u32 q0, [r11, #16]
 ; CHECK-NEXT:    ldr r6, [r1, #4]
 ; CHECK-NEXT:    vldrw.u32 q7, [r11, #32]
 ; CHECK-NEXT:    vmul.f32 q1, q1, r8
 ; CHECK-NEXT:    vmov r3, s10
 ; CHECK-NEXT:    vldrw.u32 q3, [r11, #48]
 ; CHECK-NEXT:    vfma.f32 q1, q0, r3
-; CHECK-NEXT:    ldr r3, [r1]
+; CHECK-NEXT:    ldr r3, [r1], #16
 ; CHECK-NEXT:    vfma.f32 q1, q7, r6
 ; CHECK-NEXT:    vldrw.u32 q6, [r11, #64]
 ; CHECK-NEXT:    vfma.f32 q1, q3, r3
 ; CHECK-NEXT:    vldrw.u32 q5, [r11, #80]
 ; CHECK-NEXT:    vfma.f32 q1, q6, r4
 ; CHECK-NEXT:    vldrw.u32 q4, [r11, #96]
 ; CHECK-NEXT:    vfma.f32 q1, q5, r0
 ; CHECK-NEXT:    vldrw.u32 q0, [sp, #64] @ 16-byte Reload
 ; CHECK-NEXT:    vfma.f32 q1, q4, r7
-; CHECK-NEXT:    adds r1, #16
 ; CHECK-NEXT:    vfma.f32 q1, q0, r9
 ; CHECK-NEXT:    vmov.f32 s2, s8
 ; CHECK-NEXT:    vstrb.8 q1, [r5], #16
 ; CHECK-NEXT:    le lr, .LBB19_5
 ; CHECK-NEXT:  .LBB19_6: @ %while.end
 ; CHECK-NEXT:    @ in Loop: Header=BB19_3 Depth=1
 ; CHECK-NEXT:    ldr r7, [sp, #12] @ 4-byte Reload
 ; CHECK-NEXT:    cmp r7, #0