This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add ISel patterns for scalable mask exts & truncs
ClosedPublic

Authored by frasercrmck on Jan 13 2021, 4:38 AM.

Details

Summary

Original patch by @rogfer01.

This patch adds support for sign-, zero-, and any-extension from
scalable mask vector types to integer vector types, as well as
truncation in the opposite direction.

Authored-by: Roger Ferrer Ibanez <rofirrim@gmail.com>
Co-Authored-by: Fraser Cormack <fraser@codeplay.com>

Diff Detail

Event Timeline

frasercrmck created this revision.Jan 13 2021, 4:38 AM
frasercrmck requested review of this revision.Jan 13 2021, 4:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2021, 4:38 AM
craig.topper added inline comments.Jan 13 2021, 9:45 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
516 ↗(On Diff #316366)

trunc would probably be better custom lowered. We should be able to remove the AND if the input has enough 0 bits. Not sure if SimplifyDemandedBits is capable of that for scalable vectors yet.

sext/zext/anyext might also be better custom lowered just to cut down on patterns. Our isel table is approaching 1MB. The next largest target is at about 600K.

  • replace patterns with custom-lowering
frasercrmck marked an inline comment as done.Jan 14 2021, 6:40 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
516 ↗(On Diff #316366)

Yeah, fair enough. These are good candidates for that since they're just re-expressed as standard nodes. I've done both in custom lowering functions.

craig.topper added inline comments.Jan 15 2021, 9:46 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
377

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?

1233

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?

frasercrmck marked an inline comment as done.
  • rebase on main
  • fix conflicts
frasercrmck added inline comments.Jan 18 2021, 8:30 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
377

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?

1233

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.

craig.topper added inline comments.Jan 18 2021, 10:21 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1233

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.

1251

Weren't these checked in the caller? Could we just assert here?

1255

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?

frasercrmck marked 2 inline comments as done.
  • rebase on main
  • only custom-lower for legal types
  • improve handling of illegal scalar types and splats thereof
frasercrmck marked an inline comment as not done.Jan 19 2021, 3:11 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1233

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.

1251

Yes, they were checked. I didn't fix that up when I rebased. Thanks for the spot!

1255

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.

craig.topper accepted this revision.Jan 19 2021, 10:00 AM

LGTM with that one comment.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1229

Why do we need to check that both VecVT and Src.getValueType() are vectors? Don't they always match?

This revision is now accepted and ready to land.Jan 19 2021, 10:00 AM
frasercrmck marked an inline comment as done.Jan 19 2021, 10:19 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1229

I'd sure hope so. I think I generally tend towards avoiding making assumptions but I see how that often results in redundancy. We'd be in a pickle if the types didn't match up. I'll fix that up before pushing. Cheers.

This revision was automatically updated to reflect the committed changes.
frasercrmck marked an inline comment as done.