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

[Craig Topper via Phabricator via llvm-commits]

craig.topper 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.
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117561