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

Thu Sep 17 00:48:22 PDT 2020

samparker added a comment.

Thanks, LGTM

================
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;
----------------
dmgreen wrote:
> 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.
Ah! Yes, I had completely missed the 'ARMPreAllocLoadStoreOpt::'

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

https://reviews.llvm.org/D83377