[PATCH] D72330: [ARM][MVE] Enable masked gathers from base + vector of offsets

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

This I like. I like the structure. I think it's nice to split it out like this.

I'm just not a fan of the Ty, Ptr, Mask and Alignment being part of the class. This class will be lowering multiple gathers/scatters, it may be all too easy to accidentally mis-use them in a non-obvious way, accidentally using the Mask from the wrong gather (for example). Especially as things change in the future.

If the values we need to pass around are {Ty, Ptr, Mask}, then its probably simpler to just pass them to functions that need them. Or maybe Pass `I` around? I'm not sure. The other option if we expect that there will be many parameters would be to create a struct to pass them around in. Keep them encapsulated together, but not requiring too many parameters. It looks like many of them may just be able to be grabbed from `I` quite easily though (Ty, Mask, etc).



================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:86
+
+  bool LowerGather(IntrinsicInst *I);
+  // Create a gather from a base + vector of offsets
----------------
LowerGather -> lowerGather to keep it like the others.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:163
+
+void MVEGatherScatterLowering::lookThroughBitcast() {
+  // Look through bitcast instruction if #elements is the same
----------------
This one could be `Value *MVEGatherScatterLowering::lookThroughBitcast(Value *Ptr)`, and return either `BitCast->getOperand(0)` or the original `Ptr`.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:197
   Value *Load = nullptr;
-  // Look through bitcast instruction if #elements is the same
-  if (auto *BitCast = dyn_cast<BitCastInst>(Ptr)) {
-    Type *BCTy = BitCast->getType();
-    Type *BCSrcTy = BitCast->getOperand(0)->getType();
-    if (BCTy->getVectorNumElements() == BCSrcTy->getVectorNumElements()) {
-      LLVM_DEBUG(dbgs() << "masked gathers: looking through bitcast\n");
-      Ptr = BitCast->getOperand(0);
-    }
+  if (!createMaskedGatherOffset(Load, Builder))
+    if (!createMaskedGatherBase(Load, Builder))
----------------
These could be `Value *MVEGatherScatterLowering::tryCreateMaskedGatherOffset` and return nullptr on failure. It could then look something like:
```
  Value *Load = tryCreateMaskedGatherOffset(Builder, Ty, Ptr, Mask);
  if (!Load)
    Load = tryCreateMaskedGatherBase(Builder, Ty, Ptr, Mask);
  if (!Load)
    return false;
```


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

https://reviews.llvm.org/D72330