[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/ARMBaseInstrInfo.h:359
+  // Return true if the given intrinsic is a gather or scatter
+  static bool isGatherScatter(IntrinsicInst *IntInst);
+
----------------
dmgreen wrote:
> This needn't be in the ARMBaseInstrInfo class specifically. There are some free functions at the end of this file. I think it can go with those.
Alright, I moved it there


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:392
+  // a QI gather
+  if (GEP != nullptr) {
+    Value *Load = tryCreateMergedGatherIncrement(I, BasePtr, Offsets, GEP, Builder);
----------------
dmgreen wrote:
> Can GEP ever be null here?
Now that you mention it, no it can't.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:442
+    // Look through the phi to the phi increment
+    IncrementIndex = Phi->getIncomingBlock(0) == Phi->getParent() ? 0 : 1;
+    Offsets = Phi->getIncomingValue(IncrementIndex);
----------------
dmgreen wrote:
> This should probably be using LI->getLoopLatch(), in case there are multiple blocks in the loop.
Okay.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:482-484
+  if ((P = dyn_cast<PHINode>(Add->getOperand(0))) != nullptr ||
+      ((P = dyn_cast<PHINode>(Add->getOperand(1))) != nullptr)) {
+    if (!(P->getIncomingValue(0) == Add || P->getIncomingValue(1) == Add)) {
----------------
dmgreen wrote:
> Can this be OffsetsIncoming != Phi? Or am I missing what this is checking?
Not exactly, because Phi can be nullptr in some cases where OffsetsIncoming still is a phi node. Using the latter instead of checking all operands of Add again does make this code more readable, though


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:507
+      "ScaledIndex", &Phi->getIncomingBlock(Incoming)->back());
+  Phi->setIncomingValue(Incoming, ScaledOffsets);
+  auto *ScaledOffsetsTy = cast<VectorType>(ScaledOffsets->getType());
----------------
dmgreen wrote:
> This is overwritten below I think
Yes, you are right.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:538
+  Add->eraseFromParent();
+  if (Phi != nullptr)
+    Phi->setIncomingValue(IncrementIndex, Inc);
----------------
dmgreen wrote:
> It looks like Phi is always valid here.
It is in fact.


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

https://reviews.llvm.org/D76786