[PATCH] D84732: [RISCV] Support vmsge.vx and vmsgeu.vx pseudo instructions in RVV.

[Hsiangkai Wang via Phabricator via llvm-commits]

HsiangKai added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:565
 
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
+def PseudoVMSGEU_VX : Pseudo<(outs VRegOp:$vd),
----------------
rogfer01 wrote:
> These are for the assembler parser only, I think it makes sense to mark them with `isAsmParserOnly = 1` as well.
> 
> What do you think?
It makes sense. Thanks.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:558
+
+def : InstAlias<"vmsgeu.vx $vd, $va, $rs1",
+                (PseudoVMSGEU_VX VRegOp:$vd, VRegOp:$va, GPR:$rs1), 0>;
----------------
rogfer01 wrote:
> HsiangKai wrote:
> > rogfer01 wrote:
> > > I was confused by the fact that this seems an unnecessary inst alias. However if we remove it, then `vmsgeu.vx $vd, $va, $rs1` gets parsed as `PseudoVMSGEU_VX_M` but without a mask (according to `llvm-mc --show-inst`)
> > > 
> > > ```
> > > # <MCInst #253 PseudoVMSGEU_VX_M
> > > #  <MCOperand Reg:5>
> > > #  <MCOperand Reg:6>
> > > #  <MCOperand Reg:45>
> > > #  <MCOperand Reg:0>>
> > > ```
> > > 
> > > however the pseudo `PseudoVMSGEU_VX` is explicitly unmasked.
> > > 
> > > I suggest to add a comment like this
> > > 
> > > ```
> > > // This apparently unnecessary alias prevents matching `vmsgeu.vx vd, vsrc2, rsrc1`
> > > // as if it were an umasked  (i.e. $vm = RISCV::NoRegister) PseudoVMSGEU_VX_M.
> > > ```
> > If we have no such alias. How do we match `vmsge{v}.vx v0, v1, x10`? It may match to `PseudoVMSGE_VX_M` with NoRegister mask. However, the register class for the destination operand is VRNoV0 now. There is no way to match it under the VRNoV0 register class restriction.
> Sorry I wasn't clear.
> 
> I'm not saying we don't need the `InstAlias`. We do. The reason is that masked versions need a scratch operand if the destination register is `v0`.
> 
> I suggested a comment because at a first glance the `InstAlias` seems unnecessary, as if it were only forwarding its operands to the pseudo instruction.
> 
> I'm happy if you think there is no need for such a comment here.
Sorry, I misunderstood you. It makes sense to add such comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84732