[PATCH] D83377: [ARM] Expand distributing increments to also handle existing pre/post inc instructions.

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2577
 }
+static bool isPreIndex(MachineInstr &MI) {
+  switch (MI.getOpcode()) {
----------------
samparker wrote:
> The function name doesn't appear to match that opcodes that it's checking?
Oh. Whoops!


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2688
       return false;
-    if (Use.getOperand(BaseOp + 1).getImm() == 0)
+    if (isPreIndex(Use) || isPostIndex(Use))
+      PrePostInc = &Use;
----------------
samparker wrote:
> I'm still a bit concerned with this loop.... Can't we get in a pickle if there are two or more instructions within the loop which use BaseReg with a zero offset? OtherAccesses relies on BaseAccess only being assigned once, right? Don't we also need to ensure that BaseReg isn't assigned between BaseAccess and OtherAccesses?
If I understand what you are saying properly.. We are in SSA form still, in this pre-ra pass. We shouldn't be reassigning BaseReg.

But I may have missed the point and it's been a while since I wrote this. Let me know.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83377/new/

https://reviews.llvm.org/D83377