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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked an inline comment as not done.
frasercrmck 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;
----------------
craig.topper wrote:
> Weren't these checked in the caller? Could we just assert here?
Yes, they were checked. I didn't fix that up when I rebased. Thanks for the spot!


================
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
----------------
craig.topper wrote:
> 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?
It's not well worded. It's why the code is written the way it is since it's not immediately obvious to a reader. I must have been hinting at a theoretical fix that didn't necessitate this kind of thing. Once I incorporate your above suggestion I'll remove the `FIXME`.


================
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:
> 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.
These constants are only ever sign-extended, yes. In this context it should be fine, but I was extending the scope somewhat to make it a generic scalar splat helper function. And in that case we'd have to think about how to handle difficult operands and when it gets called.

I've made the check for legality raised earlier so we can better constrain the scope of this function. The code now also only ever creates scalars of XLenVT, and does either SPLAT_VECTOR or SPLAT_VECTOR_I64 depending on the setup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94590