[PATCH] D72451: [ARM][MVE] Enable extending gathers

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:86
   // Create a gather from a base + vector of offsets
-  Value *tryCreateMaskedGatherOffset(IntrinsicInst *I, Value *Ptr,
+  Value *tryCreateMaskedGatherOffset(IntrinsicInst *I, Value *Ptr, Instruction *&Root,
                                      IRBuilder<> Builder);
----------------
This is a little long. Clang-format would fix it up I think.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:129
   if (GEPPtr->getType()->isVectorTy()) {
-    LLVM_DEBUG(dbgs() << "masked gathers: gather from a vector of pointers"
-                      << " hidden behind a getelementptr currently not"
-                      << " supported. Expanding.\n");
+    //LLVM_DEBUG(dbgs() << "masked gathers: gather from a vector of pointers"
+    //                  << " hidden behind a getelementptr currently not"
----------------
This can be left in I would guess? Or removed entirely if it's too noisy. Maybe reword it to "can't find gep" as opposed to "expanding".


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:212-218
+  Root->replaceAllUsesWith(Load);
+  Root->eraseFromParent();
+  if (Root != I)
+    // If this was an extending gather, we need to get rid of the sext/zext
+    // sext/zext as well as of the gather itself
+    I->eraseFromParent();
   LLVM_DEBUG(dbgs() << "masked gathers: successfully built masked gather\n");
----------------
If you moved this "removing" code into tryCreateMaskedGatherOffset or tryCreateMaskedGatherBase it would remove the need for a Root parameter. Not sure if it's cleaner, but it's only tryCreateMaskedGatherOffset that will have a Root, so tryCreateMaskedGatherBase's version will be simpler.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:265
+      return nullptr;
+    Root = dyn_cast<Instruction>(*I->users().begin());
+    if (isa<SExtInst>(Root)) {
----------------
This can just be cast<..>


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:266
+    Root = dyn_cast<Instruction>(*I->users().begin());
+    if (isa<SExtInst>(Root)) {
+      LLVM_DEBUG(dbgs() << "masked gathers: found an extending gather\n");
----------------
I think we have to check the type of the extend, otherwise it might be transforming to any odd thing.

Make sure there is some testcase for this too, I don't think we have maybe tests for stranger types. i64 are a good candidate. Even they are not legal (for the instruction versions that we are dealing with here), they can quite easily be created by the vectorizer.


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:284
       BasePtr->getType()->getPointerElementType()->getPrimitiveSizeInBits();
-  int ResultElemSize = Ty->getScalarSizeInBits();
+  int ResultElemSize = OriginalTy->getScalarSizeInBits();
   // This can be a 32bit load scaled by 4, a 16bit load scaled by 2, or a
----------------
ResultElemSize -> MemoryElemSize I think would be a better name. The "Result" would be after the extend.


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

https://reviews.llvm.org/D72451