[PATCH] D76786: [ARM][MVE] Add support for incrementing gathers

[Anna Welker via Phabricator via llvm-commits]

anwel added inline comments.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:512
+  PHINode *Phi = dyn_cast<PHINode>(Offsets);
+  if (Phi == nullptr || Phi->getNumIncomingValues() != 2)
+    // No phi means no IV to write back to; if there is a phi, we expect it
----------------
dmgreen wrote:
> Maybe add a check that Phi->getBB == L->getHeader too.
Has been added.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:526
+    return nullptr;
+  Value *OffsetsIncoming = Add.first;
+  int64_t Immediate = Add.second;
----------------
dmgreen wrote:
> I think we have to check that OffsetsIncoming == Phi. To make sure it's the loop structure / IV we want it to be.
Yes, you are right.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:553
+      &Phi->getIncomingBlock(1 - IncrementIndex)->back());
+  Phi->setIncomingValue(1 - IncrementIndex, OffsetsIncoming);
+
----------------
dmgreen wrote:
> As we are modifying the Phi inplace, and it can produce different values, I think we unfortunately need to check it has no other uses.
You convinced me, though it is sad we have to restrict the usage of the writeback gathers that much. I'll leave it to future work to make it right and find a way to allow them being used more often again.


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

https://reviews.llvm.org/D76786