[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:140
+    if (OriginalTy->getVectorNumElements() == 4)
+      ResultTy = VectorType::get(OriginalTy->getInt32Ty(C), 4);
+    else if (OriginalTy->getVectorNumElements() == 8)
----------------
Should this be Type::getInt32Ty ? Or Builder.getInt32() ?


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:146
+    Instruction *Parent = nullptr;
+    for (User *u : I->users()) {
+      // Only do this to gathers with exactly one use
----------------
u -> U. Or better yet just check that I->hasOneUse(). The Use of a Instruction will always be an Instruction AFAIU.

Also "Parent" makes me think of the BasicBlock/Function, not the user. Maybe just call it "User" or something like it?


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:230
     int GEPElemSize = GEP->getResultElementType()->getPrimitiveSizeInBits();
-    int ResultElemSize = Ty->getScalarSizeInBits();
+    int ResultElemSize = ResultTy->getScalarSizeInBits();
     // This can be a 32bit load scaled by 4, a 16bit load scaled by 2, or a
----------------
There are some tests in mve-gather-ind32-scaled.ll like "zext_signed_scaled_i16_i16" that I think can be converted to a `VLDRH.u32 Qd, [base, offs.sext, uxtw #1]` if we can get the extend and this scale working together. It's the type of the memory being loaded that's important for the scale, not the type of the result. (Err, I think. Check that sounds right)


================
Comment at: llvm/lib/Target/ARM/MVEGatherScatterLowering.cpp:268
+    // If this was an extending gather, also get rid of the gather instruction
+    // itself, to avoid reevaluation
+    I->eraseFromParent();
----------------
Maybe drop the "to avoid reevaluation". The rest is reason enough to remove the gather.


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

https://reviews.llvm.org/D72451