[PATCH] D96263: [RISCV] Support scalable-vector masked gather operations

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1778
+    // unsigned scaled indices it helps prevent overflow when scaling.
+    if (IndexVT.getVectorElementType().bitsLT(XLenVT)) {
+      IndexVT = IndexVT.changeVectorElementType(XLenVT);
----------------
craig.topper wrote:
> Is it possible that IndexVT has 32-bit elements on a 64-bit target and that the IndexVT is already LMUL==8 such that this SIGN_EXTEND/ZERO_EXTEND would produce an illegal LMUL==16 type?
Yes, it's possible, and similarly on a 32-bit target with an illegal 8- or 16-bit index type, but I think that implies the original intrinsic would have an illegal pointer type, like `<vscale x 16 x i32*>`. So in that sense it's ideally something LLVM would help legalize (split) for us, but sadly it folds the illegal intrinsic to one with legal types, e.g.: `i32* base, <vscale x 16 x i32> idxs` so it doesn't see that until it asks us to custom-lower.

Am I right in thinking we have to custom-legalize this ourselves, duplicating logic from `LegalizeVectorTypes`?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1788
+          !Subtarget.is64Bit() && IndexVT.getVectorElementType() == MVT::i64;
+      SDValue SplatScale = DAG.getConstant(Log2_32(N->getConstantOperandVal(5)),
+                                           DL, Subtarget.getXLenVT());
----------------
craig.topper wrote:
> Assert that this is a power of 2. I'm not completely sure that's guaranteed, but maybe it is for the types we expect to see.
Good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96263