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

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:165
 
+static DecodeStatus DecodeVRNoV0RegisterClass(MCInst &Inst, uint64_t RegNo,
+                                               uint64_t Address,
----------------
We don't want this now as we are never going to disassemble a `vmsge{u}.vx` instruction.


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

What do you think?


================
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>;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84732