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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:42
 #include "llvm/Transforms/Utils/Local.h"
+#include <experimental/optional>
 #include <algorithm>
----------------
You don't need experimental/optional (we technically need llvm/ADT/Optional.h I think, as we are using the one in llvm. It is likely included transitively at the moment).


================
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
----------------
Maybe add a check that Phi->getBB == L->getHeader too.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:517
+  unsigned IncrementIndex =
+      Phi->getIncomingBlock(0) == LI->getLoopFor(I->getParent())->getLoopLatch()
+          ? 0
----------------
Creating a variable for L sounds useful.


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


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:553
+      &Phi->getIncomingBlock(1 - IncrementIndex)->back());
+  Phi->setIncomingValue(1 - IncrementIndex, OffsetsIncoming);
+
----------------
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.


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

https://reviews.llvm.org/D76786