Page MenuHomePhabricator

[RISCV][VP] Lower VP_MERGE to RVV instructions

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



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"

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

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

Say which operand is tied


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?


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

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

frasercrmck added inline comments.Jan 20 2022, 6:39 AM

Yeah totally. I also considered making VSELECT_VL take a merge operand which means we can share a node for vp.merge, 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?


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

I think its ok to keep them separate for now.


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.



VP_MERGE_VL it is.


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.