[ARM][MVE] Teach MVEGatherScatterLowering to merge successive getelementpointers

[anwel]

Summary

A patch following up on the introduction of pointer induction variables in https://reviews.llvm.org/D81267, adding a preprocessing step to the address optimisation in the MVEGatherScatterLowering pass. If the getelementpointer that is the address is itself using a getelementpointer as base, they will be merged into one by summing up the offsets, after checking that this will not cause an overflow (this can be repeated recursively).

This is necessary as the pass can currently only handle (i.e., will produce MVE gather/scatters for) incoming addresses with a scalar base pointer.

[dmgreen]

llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp

169–1

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.

170

This function is doing quite a lot. More than just creating an offset add. Maybe call it CheckAndCreateOffsetAdd?

171

Instead of "can only zero extend", perhaps better to say "treat the offset as unsigned".

194

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.

197–198

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.

[anwel]

llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp

169–1

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?

170

That does indeed describe the purpose of the function a bit more detailed, I'm happy to go with that name.

171

Yes, makes sense.

194

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.

197–198

Okay, I simplified the if's.

[dmgreen]

llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp

169–1

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.

194

Make sure you give the whole thing a clang-format.

220–221

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.

[anwel]

llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp

169–1

So just putting in a safeguard to make sure our assumptions are correct - yes, makes sense. I added it.

220–221

Less lines is always good :)

[dmgreen]

Thanks. LGTM