[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:200
+      int SExtValue = OConst->getSExtValue();
+    if (SExtValue >= TargetElemMaxSize || SExtValue < 0)
+        return false;
----------------
Make sure you give the whole thing a clang-format.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:1083-1084
+      ConstantInt *ConstYEl;
+      if ((ConstXEl = dyn_cast<ConstantInt>(ConstX->getAggregateElement(i))) &&
+          (ConstYEl = dyn_cast<ConstantInt>(ConstY->getAggregateElement(i))) &&
+          ConstXEl->getZExtValue() + ConstYEl->getZExtValue() <
----------------
You can pull these dyn_casts out the if too, although I'm not sure it makes a bit difference here. It should end up with less lines, at least.

I would also invert the meaning of this if. if (!ConstXEl || !ConstYEl || ...) return nullptr; Then you don't need the continue. Up to you though.


================
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);
----------------
anwel wrote:
> 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?
The idea would be that somewhere early, perhaps in the `if (II && II->getIntrinsicID() == Intrinsic::masked_gather) {`, we would check if it's a fixed vector and if not ignore that gather. That way we would know that all the other vectors we are dealing with are FixedVectorType's.


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

https://reviews.llvm.org/D84027