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

[Tim Northover]

t.p.northover accepted this revision.
t.p.northover added a reviewer: t.p.northover.
t.p.northover added a comment.

I didn't spot anything horribly wrong (certainly nothing newly horribly wrong). The only thing that really needs fixing now is the dead code, which you can probably just remove.


================
Comment at: lib/Target/ARM/ARMLoadStoreOptimizer.cpp:353
@@ -350,1 +352,3 @@
 
+static bool isLoad(unsigned Opc) {
+  return isi32Load(Opc) || Opc == ARM::VLDRS || Opc == ARM::VLDRD;
----------------
Maybe isLaodSingle or something? The name seems a bit too generic for what it's actually doing.

================
Comment at: lib/Target/ARM/ARMLoadStoreOptimizer.cpp:413
@@ +412,3 @@
+  case ARM::VSTMDIA:
+    return (MI->getNumOperands() - MI->getDesc().getNumOperands() + 1) * 8;
+  }
----------------
Looks dodgy, but I see you just moved this function around.

I've not quite managed to produce an example, but I'd be entirely unsurprised to see something like this used to load a Q-reg:

    VLDMDIA %R0, %D0<def-dead>, %D1<def-dead>, %Q0<imp-def>

================
Comment at: lib/Target/ARM/ARMLoadStoreOptimizer.cpp:787
@@ -745,1 +786,3 @@
 
+#if 0
+  // TODO FIXME: Look at this
----------------
Commented code. Actually, commented code that I added, I think. It was written to prevent:

    add rNewBase, rBase<kill>, #12
    stm rNewBase, {..., rBase, ...}

where the Base was killed prematurely if it ended up actually being used in the store. This caused "-verify-machineinstr" failures, but nothing else that I've observed.

It looks like your code aroung line 654 replaces it adequately though.


Repository:
  rL LLVM

http://reviews.llvm.org/D10140