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

[Anna Welker via Phabricator via llvm-commits]

anwel 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);
----------------
dmgreen wrote:
> 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.
I made sure to use FixedVectorType everywhere so it's more uniform. Do you think it is really necessary to check somewhere that we are actually dealing with these? We currently are casting to FixedVectorType almost everywhere in this pass without any safeguards, including the type of the gather or the input of the scatter itself, or of the address, or parts of the address - if we have to safeguard any of these, then why not the others?


================
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
----------------
dmgreen wrote:
> Instead of "can only zero extend", perhaps better to say "treat the offset as unsigned".
Yes, makes sense.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:200
+                  dyn_cast<ConstantInt>(ConstOff->getAggregateElement(i))) ||
+            (OConst->getZExtValue() >= (unsigned)(1 << (TargetElemSize - 1))))
+          return false;
----------------
dmgreen wrote:
> 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.
You are right. I have replaced this by using a signed extend and checking that the resulting  value is neither negative nor bigger than allowed, and also moved those lines into a helper function so they're not being duplicated.


================
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;
----------------
dmgreen wrote:
> 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.
Okay, I simplified the if's.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:1025
 
+Value *CreateOffsetAdd(Value *X, Value *Y, Value *GEP, IRBuilder<> &Builder) {
+
----------------
dmgreen wrote:
> This function is doing quite a lot. More than just creating an offset add. Maybe call it CheckAndCreateOffsetAdd?
That does indeed describe the purpose of the function a bit more detailed, I'm happy to go with that name.


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

https://reviews.llvm.org/D84027