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

[Anna Welker via Phabricator via llvm-commits]

anwel marked 4 inline comments as done.
anwel added inline comments.


================
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
----------------
dmgreen wrote:
> 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?
Okay. I wasn't sure about whether users always have to be instructions. If they are, the Parent variable becomes superfluous.


================
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
----------------
dmgreen wrote:
> 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)
You're right. The modularisation fixed whatever was keeping those tests from being converted to a gather, they look better now.


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

https://reviews.llvm.org/D72451