[PATCH] D77813: [ARM] Distribute MVE post-increments

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2538
+// postinc writeback.
+static int getAddSubOffset(MachineInstr &MI) {
+  switch (MI.getOpcode()) {
----------------
samparker wrote:
> Looks like these helpers should be extracted to ARMBaseInstrInfo, I also have a helper in ARMLowOverheadLoops to get this immediate. And I would have thought that there's already something to legal addressing modes somewhere?
I haven't seen anything for addressing modes exactly. They all seems to be spread over ARMISelDAGToDAG, if anywhere.

I'll try and move what I can. The getBaseOperandIndex above is really "shouldTryDistributePostInc" more than anything else, so I've left it here. There appears to be another one for getting add/sub offsets in this very file, but is also testing other things to do with reg usages and predicates.


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2616
+    if (getAddSubOffset(Use) != 0)
+      Increment = &Use;
+    else if ((BaseOp = getBaseOperandIndex(Use)) != -1) {
----------------
samparker wrote:
> Could increment be assigned multiple times? And if so, does that matter?
Hmm. I remember thinking about that and figuring it was OK. Every time I tried, MRI->use_nodbg_instructions seemed to be in order, so I think it would be OK. I'm not sure if that is really guaranteed though.

I'll add a check to make sure there is only one. We can always make it more relaxed in the future if needed.


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2670
+      }
+    } else if (!DT->dominates(Use, BaseAccess)) {
+      LLVM_DEBUG(
----------------
samparker wrote:
> Do we need this condition..?
Like I said above in the comment this is to be on the safe side, and only handle instructions where either BaseAccess dominates Use or Use dominated BaseAccess.


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

https://reviews.llvm.org/D77813