[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:
> frasercrmck wrote:
> > frasercrmck wrote:
> > > 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`?
> > It should be possible to do a DAGCombine pre-legalization which creates the illegal XLEN index types and passes it back to go through the vanilla legalization, if you think that's a valid route.
> > 
> > Either way it's going to require support for scalable-vector insert/extract subvector. In the worst case we'll have to expand nxv64i64 (from nxv64i8) into 8 x nxv8i64.
> Would the DAG combine end up fighting with the combine that uses shouldRemoveExtendFromGSIndex?
It does, so you have to turn off that hook. I don't think that's the worst thing for our gathers since only UNSIGNED UNSCALED can be represented as index types smaller than XLEN, and I couldn't find anything that would generate that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96263