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

[James Molloy]

Hi Mattias,

Sorry it took so long for me to get to this, for some reason Phab's mail didn't go direct to my inbox but got buried in list mail.

I have some comments, some of them are just me being dim, needing some enlightenment :)

Cheers,

James


REPOSITORY
  rL LLVM

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

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

================
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))
----------------
Does/should this respect the AllocationOrder for RegClass?

================
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) {
----------------
I think it's ambiguous what "Before" means here, given it operates in reverse order. Is this before-when-going-back-to-front?

================
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);
----------------
Any particular reason you upped this limit?

================
Comment at: lib/Target/ARM/ARMLoadStoreOptimizer.cpp:667
@@ -666,3 @@
-
-  // Add implicit defs for super-registers.
-  for (unsigned ImpDef : ImpDefs)
----------------
Why has this been removed?

http://reviews.llvm.org/D10140

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