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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked 6 inline comments as done.
frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.h:253
 
+  // Vector select with a tied operand and additional VL operand. This
+  // operation is unmasked.
----------------
craig.topper wrote:
> Say which operand is tied
Done.


================
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:
> frasercrmck wrote:
> > 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? 
> I think its ok to keep them separate for now.
`VP_MERGE_VL` it is.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:1085
 
+class VPseudoTiedBinaryCarryIn<VReg RetClass,
+                               VReg Op1Class,
----------------
craig.topper wrote:
> frasercrmck wrote:
> > 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?
> 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.
Perfect yeah, thanks! Only thing is I'm not really sure what to call `VPseudoTiedBinaryCarryIn`. Maybe it's fine as it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117561