[PATCH] ARMLoadStoreOptimizer: Rewrite LDM/STM matching logic.

[Matthias Braun]

================
Comment at: lib/Target/ARM/ARMLoadStoreOptimizer.cpp:101
@@ +100,3 @@
+      SmallVector<MachineInstr*, 4> Instrs;
+      unsigned LatestMIIdx;
+      unsigned EarliestMIIdx;
----------------
jmolloy wrote:
> What does the "Idx" refer to? Position from the end of a block?
I added a comment:

```
/// Index in Instrs of the instruction being latest in the schedule.
```

================
Comment at: lib/Target/ARM/ARMLoadStoreOptimizer.cpp:103
@@ +102,3 @@
+      unsigned EarliestMIIdx;
+      unsigned InsertPos;
+    };
----------------
jmolloy wrote:
> Again, from the back or front of the block? You've mentioned reverse position in the above struct, so disambiguation would be good here.
I added a comment:

```
/// Index into the basic block where the merged instruction will be
/// inserted. (See MemOpQueueEntry.Position)
```

================
Comment at: lib/Target/ARM/ARMLoadStoreOptimizer.cpp:514
@@ +513,3 @@
+ARMLoadStoreOpt::findFreeReg(const TargetRegisterClass &RegClass) const {
+  for (unsigned Reg : RegClass)
+    if (!MRI->isReserved(Reg) && !LiveRegs.contains(Reg))
----------------
jmolloy wrote:
> Does/should this respect the AllocationOrder for RegClass?
I had modeled this after the behaviour of the RegisterScavenger, but you are right taking the allocation order into account is a good thing; I changed it to use the allocation order.

================
Comment at: lib/Target/ARM/ARMLoadStoreOptimizer.cpp:522
@@ +521,3 @@
+/// backwards so multiple queries in the same block must come in reverse order.
+void ARMLoadStoreOpt::moveLiveRegsBefore(const MachineBasicBlock &MBB,
+    MachineBasicBlock::const_iterator Before) {
----------------
jmolloy wrote:
> I think it's ambiguous what "Before" means here, given it operates in reverse order. Is this before-when-going-back-to-front?
Hmm I always have the problem that you often see code with a "position" pointing at an instruction and a set of registers that are live at that instruction. However that is highly ambiguous since you can only talk about live register immediately before or after an instruction, so when I write code I always use "before" or "after" when talking about liveness at an instruction. Anyway I changed the comment to resolve the ambiguity.

================
Comment at: lib/Target/ARM/ARMLoadStoreOptimizer.cpp:562
@@ -497,3 +561,3 @@
   bool SafeToClobberCPSR = !isThumb1 ||
-    (MBB.computeRegisterLiveness(TRI, ARM::CPSR, std::prev(MBBI), 15) ==
+    (MBB.computeRegisterLiveness(TRI, ARM::CPSR, InsertBefore, 20) ==
      MachineBasicBlock::LQR_Dead);
----------------
jmolloy wrote:
> Any particular reason you upped this limit?
We didn't hit one case in a lit-test anymore and this fixed it, I think this is because we now use the "InsertBefore" instead of the "InsertAfter" position.

================
Comment at: lib/Target/ARM/ARMLoadStoreOptimizer.cpp:667
@@ -666,3 @@
-
-  // Add implicit defs for super-registers.
-  for (unsigned ImpDef : ImpDefs)
----------------
jmolloy wrote:
> Why has this been removed?
This is still happening but as part of MergeOpsUpdate() now around line 831.

http://reviews.llvm.org/D10140

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/