This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][VP] Lower VP_MERGE to RVV instructions
ClosedPublic

Authored by frasercrmck on Jan 18 2022, 6:35 AM.

Details

Summary

This patch adds lowering of the llvm.vp.merge.* intrinsic
(ISD::VP_MERGE) to RVV vmerge/vfmerge instructions. It introduces a
special pseudo form of vmerge which allows a tied merge operand,
allowing us to specify the tail elements as being equal to the "on
false" operand, using a tied-def constraint and a "tail undisturbed"
policy.

While this strategy allows us to often lower the intrinsic to just one
instruction, it may be less efficient in fixed-vector types as the
number of tail elements may extend far beyond the length of the fixed
vector. Another strategy could be to use a vmerge/vfmerge instruction
with an AVL equal to the length of the vector type, and manipulate the
condition operand such that mask elements greater than the operation's
EVL are false.

I've also observed inefficient codegen in which our 'VF' patterns don't
match raw floating-point SPLAT_VECTORs, which occur in scalable-vector
code.

Diff Detail

Event Timeline

frasercrmck created this revision.Jan 18 2022, 6:35 AM
frasercrmck requested review of this revision.Jan 18 2022, 6:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2022, 6:35 AM
craig.topper added inline comments.Jan 19 2022, 3:50 PM
llvm/lib/Target/RISCV/RISCVISelLowering.h
254

Say which operand is tied

256

This is an unfortunate name collision with the RVV instruction. While also having weird constraints. Maybe VPMERGE_VL or VP_MERGE_VL would be slightly better?

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
1086

Does it really need to tied or can we have a _TU instruction and pass the false value to two operands in the pattern match?

craig.topper added inline comments.Jan 19 2022, 3:55 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
1086

Let me rephrase that. We obviously need an operand that is tied. But it doesn't need to be the same as $rs2. We can assign $rs2 and the merge operand to the same src in the pattern.

But I guess we don't have the concept of _TU instructions until https://reviews.llvm.org/D117647

frasercrmck added inline comments.Jan 20 2022, 6:39 AM
llvm/lib/Target/RISCV/RISCVISelLowering.h
256

Yeah totally. I also considered making VSELECT_VL take a merge operand which means we can share a node for vp.merge, vp.select and vselect. I don't know if that's too intrusive into areas where a merge operand doesn't make sense, though. We'd probably need a policy operand, too... What do you think?

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
1086

Yeah that would work too. Are we really dependent on that patch or can we have a HasMergeOp = 1 in conjunction HasVecPolicyOp = 1 where we pass along TAIL_UNDISTURBED? Or is it then that we don't really have a suffix to convey that: _TIED isn't quite right and _MASK isn't either since vmerge isn't "masked", in that sense?

craig.topper added inline comments.Jan 20 2022, 10:03 AM
llvm/lib/Target/RISCV/RISCVISelLowering.h
256

I think its ok to keep them separate for now.

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
1086

I think you can use _TU and take the change PseudoToVInst from D117647

Add the extra tied source operand and use HasMergeOp = 1 and HasVecPolicyOp = 0. We don't need a policy op since the tied source being undef can distinquish tail undisturbed vs agnostic.

use a _TU suffix (taken from D117647)
drop the policy op: use an explicit tied merge/passthru operand
rename to VP_MERGE_VL: document node accordingly

frasercrmck marked 6 inline comments as done.Jan 21 2022, 7:09 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.h
254

Done.

256

VP_MERGE_VL it is.

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
1086

Perfect yeah, thanks! Only thing is I'm not really sure what to call VPseudoTiedBinaryCarryIn. Maybe it's fine as it is.

This revision is now accepted and ready to land.Jan 21 2022, 11:01 AM
This revision was automatically updated to reflect the committed changes.
frasercrmck marked 3 inline comments as done.