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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:162
+    if (OffsType != Offsets->getType()) {
+      if (OffsType->getScalarSizeInBits() >=
+          Offsets->getType()->getScalarSizeInBits()) {
----------------
This can be ">"


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:176
+    Value *GEPPtr = GEP->getPointerOperand();
+    if (!GEPPtr->getType()->isVectorTy())
+      BasePtr = GEPPtr;
----------------
Do we have and tests where this is false? Is it for something like `getelementptr <4 x i32>, <4 x i32> *%base, i64 16`? That might make sense to generate a QI gather load from (although I don't know if anything will generate gep's like that at the moment without some help).

For the RQ form, we need to make sure that base is a single pointer.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:181
+    // Ensure all the other indices are 0.
+    for (unsigned i = 1; i < FinalIndex; ++i) {
+      auto *C = dyn_cast<Constant>(GEP->getOperand(i));
----------------
This code comes from the ISel gather lowering? I think that for the moment we can just check that the number of GEP operands is 2 (Base + offset) and remove this loop. I'm not exactly sure when it would be useful. We don't seem to have any tests for it, and can add it back in later if they come up in the future. (Otherwise the types don't sound like they would be correct, and the getOperand(1) should be getOperand(FinalIndex)?)


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:189
+    LLVMContext &C = I->getContext();
+    if (BasePtr->getType() == Type::getInt32PtrTy(C)
+        && Ty->getScalarSizeInBits() == 32) {
----------------
I think this can be something like GEP->getResultElementType()->getPrimitiveSizeInBits() == 32. Same below, so maybe make a variable for that size. (scaled_v8f16_half should be be matched too, I think).

Also Clang format!


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:198
+    } else {
+      return false;
+    }
----------------
Worth adding a LLVM_DEBUG message so we can tell what prevented the generation.


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

https://reviews.llvm.org/D72330