[PATCH] D84027: [ARM][MVE] Teach MVEGatherScatterLowering to merge successive getelementpointers

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:87
   // argument
-  Value *checkGEP(Value *&Offsets, Type *Ty, GetElementPtrInst *GEP,
+  Value *checkGEP(Value *&Offsets, VectorType *Ty, GetElementPtrInst *GEP,
                   IRBuilder<> &Builder);
----------------
It might be worth putting something in very early in the pass that checks that the type is a FixedVectorType, and if not just bails. It would allow us to know we are only dealing with fixed vectors everywhere in the pass.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:177
+  // Offsets that are not of type <N x i32> are sign extended by the
+  // getelementptr instruction, and MVE gathers/scatters can only zero extend.
+  // Thus, if the element size is smaller than 32, we can only allow positive
----------------
Instead of "can only zero extend", perhaps better to say "treat the offset as unsigned".


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:200
+                  dyn_cast<ConstantInt>(ConstOff->getAggregateElement(i))) ||
+            (OConst->getZExtValue() >= (unsigned)(1 << (TargetElemSize - 1))))
+          return false;
----------------
I think in some cases the limit can be 1 << TargetElemSize, if the type is large enough. But for small types you may have to do something like using getSExtValue, which would then mean you need to check >= 0 too.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:203-204
+    } else {
+      if (!((OConst = dyn_cast<ConstantInt>(ConstOff)) &&
+            OConst->getZExtValue() >= (unsigned)(1 << (TargetElemSize - 1))))
+        return false;
----------------
These if's might be a little simpler if OConst was pulled out of them. Like:
  ConstantInt *OConst = dyn_cast<ConstantInt>(ConstOff);
  if (!OConst || OConst->getZExtValue() >= (unsigned)(1 << (TargetElemSize - 1)))
I guess the 1 << (TargetElemSize - 1) could be calculated earlier.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:1025
 
+Value *CreateOffsetAdd(Value *X, Value *Y, Value *GEP, IRBuilder<> &Builder) {
+
----------------
This function is doing quite a lot. More than just creating an offset add. Maybe call it CheckAndCreateOffsetAdd?


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

https://reviews.llvm.org/D84027