This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][VP] Custom lower VP_SCATTER and VP_GATHER
ClosedPublic

Authored by frasercrmck on Aug 31 2021, 4:57 AM.

Details

Summary

This patch adds support for the VP_SCATTER and VP_GATHER nodes by
lowering them to RVV's vsox/vlux instructions, respectively. This
process is almost identical to the existing MSCATTER/MGATHER support.

One extra change was made to SelectionDAGLegalize so that
VP_SCATTER's operation action is derived from its stored "value"
operand rather than its return type (which is always the chain).

Diff Detail

Event Timeline

frasercrmck created this revision.Aug 31 2021, 4:57 AM
frasercrmck requested review of this revision.Aug 31 2021, 4:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2021, 4:57 AM
frasercrmck retitled this revision from [RISCV] Custom lower VP_SCATTER and VP_GATHER to [RISCV][VP] Custom lower VP_SCATTER and VP_GATHER.Aug 31 2021, 5:02 AM
rogfer01 accepted this revision.Sep 1 2021, 11:27 PM

LGTM! Thanks @frasercrmck !

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

I'd say so. Using ISD::SHL causes the emission of a vsetvli to switch to vlmax temporarily. I understand we can do this in a later patch as it seems a low hanging fruit.

(That said I understand this as part of a deeper problem by itself: geps used to synthesize the vector of pointers fed to vp.{gather,scatter} will be already be broken down by DAGBuilder in vlmax operations. My wild guess here is that we need something like "demandedvectorelements" but aware of "vlmax" vs "not vlmax").

This revision is now accepted and ready to land.Sep 1 2021, 11:27 PM
craig.topper added inline comments.Sep 1 2021, 11:43 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4724

Couldn't this have been a dyn_cast and avoid the IsVP parameter?

4826

Same here.

6453

Could we just dyn_cast<VPScatterSDNode> and avoiding needing IsVP?

  • rebase & remove various IsVPs
frasercrmck marked 3 inline comments as done and an inline comment as not done.Sep 2 2021, 2:06 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4724

Yeah, I was in two minds about that. Originally I wasn't as big of a fan of dyn_cast followed by assert/cast - at least IsVP allowed us to assert/cast on each, requiring the user to pass the right thing. But then that's pretty cumbersome. I've removed the IsVPs and I think it looks alright.

6446

Yeah you're right: there's quite a lot going on which gets in our way. Even the SIGN_EXTEND/ZERO_EXTEND above will switch to vlmax and I don't think we have a way of solving that. Except, as you suggest, using some clever analysis of users. If all users of an instruction foo use a VL of X (or a max of X if you can infer that at compile time) then you're probably pretty safe to reduce the VL of that instruction foo down to X. Though I've only considered that for a few seconds so might have missed something :)

I'm not sure "where" that could get done. It's somewhat analogous to the W instructions, isn't it?

6453

Yeah, here IsVP makes less sense. There's already a precedent set by the dyn_cast below.

frasercrmck marked 2 inline comments as done and an inline comment as not done.Sep 3 2021, 5:54 AM

Sorry, I seem to have forgotten everything about phabricator process - I wasn't sure if you wanted to approve after I addressed your suggestions, @craig.topper, or whether approval was implied by Roger's.

craig.topper added inline comments.Sep 3 2021, 9:59 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4747

Can we share MemVT, MMO, Chain, and BasePtr initializing by using the MemSDNode base?

4760

Should this comment be above the cast?

4827

Same question about MemSDNode

6406

dyn_cast in the above if?

  • use common memsdnode to initialize some variables
  • rebase on baseptr fix
frasercrmck marked 4 inline comments as done.Sep 6 2021, 2:54 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4747

Indeed we can. Thanks. Earlier I was wondering if VP_GATHER & MGATHER and VP_SCATTER & MSCATTER could share an intermediate base class to simplify this code even further or whether that's introducing too deep a hierarchy. Anyway maybe that come later, though I'd like to hear your thoughts.

4760

hah yep.

6406

Yeah can do. I'm now using a dyn_cast. I originally kept it as an opcode check because I thought it kept it logically a bit closer to the switch statement.

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