[PATCH] D94590: [RISCV] Add ISel patterns for scalable mask exts & truncs

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:374
+      // Custom-lower extensions and truncations to/from mask types
+      setOperationAction(ISD::ANY_EXTEND, VT, Custom);
+      setOperationAction(ISD::SIGN_EXTEND, VT, Custom);
----------------
craig.topper wrote:
> Technically this should only be for the legal types. But I guess we don't expect to see scalable vectors that need to be type legalized?
Not at this stage. I agree it would be better, though. Having an array or vector of legal vector types would be useful, to start with. Should I fold that into this patch or clean it up later?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1178
+
+  // FIXME: Be careful about splatting constants, as getConstant can't
+  // legalize vXi64 on RV32
----------------
craig.topper wrote:
> I don't think we should be creating scalars with MVT::i64 type on RV32 here. It will usually work because a round of type legalization is called after vector op legalization. Which will legalize the ISD::SPLAT_VECTOR. But if for some reason a mask extension is ever introduced during LegalizeDAG, the scalar type wouldn't get legalized. Can we just detect this case and emit SPLAT_VECTOR_I64 here?
True. It's quite fiddly but I'm not sure we can avoid that. I'm a bit concerned that if we wanted to do this generically we can't always use SPLAT_VECTOR_I64 since the scalar won't always be (provably) a sign-extended 32-bit value. Also I don't think there's a way of detecting the level we're at, so we'd be creating SPLAT_VECTOR_I64 quite early on, or expanding the sequence for non-sign-extended values, which might inhibit generic SPLAT_VECTOR combines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94590