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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1247
+  // Only custom-lower truncations to mask types
+  if (!MaskVT.isVector() || MaskVT.getVectorElementType() != MVT::i1)
+    return Op;
----------------
Weren't these checked in the caller? Could we just assert here?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1251
+  EVT VecVT = Src.getValueType();
+  // FIXME: Be careful about splatting constants, as getConstant can't
+  // legalize vXi64 on RV32
----------------
Is the FIXME telling me why the code is written then way it is? Or is trying to tell me we need to make additional changes to be careful?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1178
+
+  // FIXME: Be careful about splatting constants, as getConstant can't
+  // legalize vXi64 on RV32
----------------
frasercrmck wrote:
> 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.
The constants used here are sign extended though right? You are correct there's no way to tell what level we're at in Lowering. Only DAG combine has a level we can check.

Assuming the type is legal, this code will first run in LegalizeVectorOps. It will produce a SPLAT_VECTOR. Then we'll immediately run scalar type legalization which will turn it into SPLAT_VECTOR_I64. I don't think there's a DAG combine in between.

I think the only optimizations that will have an opportunity to see the SPLAT_VECTOR would be in LegalizeVectorOps. If I remember right, that visits operands before users. (LegalizeDAG is the opposite). So if we do what I suggested, I think we would make the SPLAT_VECTOR_I64 and the VSELECT before the users of the VSELECT are visited. So if the legalization of the user of the VSELECT did some optimization that looked through the VSELECT and found the unknown SPLAT_VECTOR_I64 instead of SPLAT_VECTOR then whatever optimization it was trying to do would break. Not sure how likely that is, legalization doesn't typically look very deep through operands beyond maybe calling computeKnownBits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94590