[ARM] LoadStoreOptimizer: reoder limit

[SjoerdMeijer]

Summary

Today I was a bit distracted by the loadstoreoptimiser because it didn't do
what I wanted/expected.

This patch adds a (hidden) option to control the total number of instructions that
can be re-ordered. I appreciate this looks only a tiny bit better than a hard-coded
constant, but at least it allows more easy experimentation with different values
for now. Ideally we calculate this reorder limit based on some heuristics. I might be
looking into that next.

Second patch D57955 is just a simple clean up.

[efriedma]

I guess this is fine. LGTM

Ultimately, the whole design here is sort of flaky: the scheduling code here isn't aware of register pressure, and the register allocator isn't aware of the possibility of generating ldm/stm, so the end result is very unreliable. I'm not sure what the right solution is, but tweaking the scheduling heuristics isn't going to help much.

[SjoerdMeijer]

Thanks Eli, also for your thoughts on this:

Ultimately, the whole design here is sort of flaky: the scheduling code here isn't aware of register pressure, and the register allocator isn't aware of the possibility of generating ldm/stm, so the end result is very unreliable.

Unreliable results is exactly what I was seeing, and that's why I started studying first what exactly this loadstoreoptimiser is doing.

I'm not sure what the right solution is, but tweaking the scheduling heuristics isn't going to help much.

Agreed. Also sounds a little bit like a can of worms. I need to first investigate more where exactly my problems are caused, depending on that I might be looking more into this. Making this rescheduling register pressure aware might be a good step in the right direction? But as you also said, not the complete answer...

llvm/trunk/lib/Target/ARM/ARMLoadStoreOptimizer.cpp

 
 } // end anonymous namespace
 
 char ARMPreAllocLoadStoreOpt::ID = 0;
 
 INITIALIZE_PASS(ARMPreAllocLoadStoreOpt, "arm-prera-ldst-opt",
                 ARM_PREALLOC_LOAD_STORE_OPT_NAME, false, false)
 
+// Limit the number of instructions to be rescheduled.
+// FIXME: tune this limit, and/or come up with some better heuristics.
+static cl::opt<unsigned> InstReorderLimit("arm-prera-ldst-opt-reorder-limit",
+                                          cl::init(8), cl::Hidden);
+
 bool ARMPreAllocLoadStoreOpt::runOnMachineFunction(MachineFunction &Fn) {
   if (AssumeMisalignedLoadStores || skipFunction(Fn.getFunction()))
     return false;
 
   TD = &Fn.getDataLayout();
   STI = &static_cast<const ARMSubtarget &>(Fn.getSubtarget());
   TII = STI->getInstrInfo();
   TRI = STI->getRegisterInfo();
for (int i = Ops.size() - 1; i >= 0; --i) {
       int Offset = getMemoryOpOffset(*Op);
       unsigned Bytes = getLSMultipleTransferSize(Op);
       if (LastBytes) {
         if (Bytes != LastBytes || Offset != (LastOffset + (int)Bytes))
           break;
       }
 
       // Don't try to reschedule too many instructions.
-      if (NumMove == 8) // FIXME: Tune this limit.
+      if (NumMove == InstReorderLimit)
         break;
 
       // Found a mergable instruction; save information about it.
       ++NumMove;
       LastOffset = Offset;
       LastBytes = Bytes;
       LastOpcode = LSMOpcode;
 

llvm/trunk/test/CodeGen/ARM/prera-ldst-insertpt.mir

 # RUN: llc -run-pass arm-prera-ldst-opt %s -o - | FileCheck %s
+# RUN: llc -run-pass arm-prera-ldst-opt -arm-prera-ldst-opt-reorder-limit=3 %s -o - | FileCheck %s
+# RUN: llc -run-pass arm-prera-ldst-opt -arm-prera-ldst-opt-reorder-limit=2 %s -o - | FileCheck %s --check-prefix=CHECK-LIMIT
 --- |
   target triple = "thumbv7---eabi"
 
   define void @a(i32* nocapture %x, i32 %y, i32 %z) {
   entry:
     ret void
   }
 
bb.0.entry:
     t2STRi12 %1, %0, 0, 14, $noreg :: (store 4)
     %10 : rgpr = t2LSLri %2, 1, 14, $noreg, $noreg
     t2STRi12 killed %10, %0, 4, 14, $noreg :: (store 4)
     %3 : rgpr = t2MUL %2, %2, 14, $noreg
     t2STRi12 %3, %0, 8, 14, $noreg :: (store 4)
 
     ; Make sure we move the paired stores next to each other, and
     ; insert them in an appropriate location.
-    ; CHECK: t2STRi12 {{.*}}, 0
+    ; CHECK:      t2STRi12 {{.*}}, 0
     ; CHECK-NEXT: t2STRi12 {{.*}}, 4
     ; CHECK-NEXT: t2STRi12 {{.*}}, 8
     ; CHECK-NEXT: t2MUL
     ; CHECK-NEXT: t2MOVi32imm
 
+    ; CHECK-LIMIT-LABEL: name: b
+    ; CHECK-LIMIT:      t2STRi12 {{.*}}, 0
+    ; CHECK-LIMIT-NEXT: t2STRi12 {{.*}}, 4
+    ; CHECK-LIMIT-NEXT: t2MUL
+    ; CHECK-LIMIT-NEXT: t2STRi12 {{.*}}, 8
+
     %4 : rgpr = t2MUL %1, %1, 14, $noreg
     %5 : rgpr = t2MOVi32imm -858993459
     %6 : rgpr, %7 : rgpr  = t2UMULL killed %3, %5, 14, $noreg
     %8 : rgpr, %9 : rgpr  = t2UMULL killed %4, %5, 14, $noreg
     %10 : rgpr = t2LSLri %2, 1, 14, $noreg, $noreg
     %11 : rgpr = t2MOVi 55, 14, $noreg, $noreg
     %12 : gprnopc = t2ADDrs %11, killed %7, 19, 14, $noreg, $noreg
     t2STRi12 killed %12, %0, 16, 14, $noreg :: (store 4)