[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:1083-1084
+      ConstantInt *ConstYEl;
+      if ((ConstXEl = dyn_cast<ConstantInt>(ConstX->getAggregateElement(i))) &&
+          (ConstYEl = dyn_cast<ConstantInt>(ConstY->getAggregateElement(i))) &&
+          ConstXEl->getZExtValue() + ConstYEl->getZExtValue() <
----------------
dmgreen wrote:
> 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.
Less lines is always good :)


================
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:
> 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.
So just putting in a safeguard to make sure our assumptions are correct - yes, makes sense. I added it.


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

https://reviews.llvm.org/D84027