[PATCH] D117561: [RISCV][VP] Lower VP_MERGE to RVV instructions

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.h:255
+  // operation is unmasked.
+  VMERGE_VL,
   // Vector select with an additional VL operand. This operation is unmasked.
----------------
craig.topper wrote:
> 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?
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? 


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:1085
 
+class VPseudoTiedBinaryCarryIn<VReg RetClass,
+                               VReg Op1Class,
----------------
craig.topper wrote:
> craig.topper wrote:
> > 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?
> 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
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117561