This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support vmsge.vx and vmsgeu.vx pseudo instructions in RVV.
ClosedPublic

Authored by HsiangKai on Jul 28 2020, 1:11 AM.

Details

Summary

Implement vmsge{u}.vx pseudo instruction.

According to RISC-V V specification, there are different scenarios for this pseudo instruction. I list them below.

unmasked va >= x

  pseudoinstruction: vmsge{u}.vx vd, va, x
  expansion: vmslt{u}.vx vd, va, x; vmnand.mm vd, vd, vd

masked va >= x, vd != v0

  pseudoinstruction: vmsge{u}.vx vd, va, x, v0.t
  expansion: vmslt{u}.vx vd, va, x, v0.t; vmxor.mm vd, vd, v0

masked va >= x, vd == v0

  pseudoinstruction: vmsge{u}.vx vd, va, x, v0.t, vt
  expansion: vmslt{u}.vx vt, va, x;  vmandnot.mm vd, vd, vt

Use pseudo instruction to model vmsge{u}.vx. The pseudo instruction will convert to different expansion according to the condition.

Diff Detail

Event Timeline

HsiangKai created this revision.Jul 28 2020, 1:11 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 28 2020, 1:11 AM
Harbormaster failed remote builds in B65972: Diff 281134!
HsiangKai updated this revision to Diff 281139.Jul 28 2020, 1:30 AM

Fix problems when rebasing.

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 28 2020, 2:00 AM
Harbormaster failed remote builds in B65973: Diff 281139!
HsiangKai updated this revision to Diff 281153.Jul 28 2020, 2:28 AM

Apply clang-format.

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 28 2020, 3:14 AM
Harbormaster failed remote builds in B65979: Diff 281153!
HsiangKai requested review of this revision.Aug 6 2020, 8:08 PM
rogfer01 added inline comments.Aug 10 2020, 11:10 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
233

This assertion can be triggered by assembling the following instruction using llvm-mc --triple riscv64 -mattr +experimental-v --filetype=obj

vmsge.vx v0, v1, x10, v0.t

perhaps we could consider having a specific operand VRegOpNoV0 (or similar) which rejects v0 here?

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
586

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.
588

And a comment here, something like

See comment above. This avoids matching an unmasked `PseudoVMSGE_VX_M`

Add VRNoV0 register class.

HsiangKai added inline comments.Aug 23 2020, 10:59 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
233

I added a new register class for it and added test cases.

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
586

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.

rogfer01 added inline comments.Sep 1 2020, 11:13 PM
llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
165

We don't want this now as we are never going to disassemble a vmsge{u}.vx instruction.

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
565

These are for the assembler parser only, I think it makes sense to mark them with isAsmParserOnly = 1 as well.

What do you think?

586

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.

HsiangKai added inline comments.Sep 9 2020, 6:44 PM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
565

It makes sense. Thanks.

586

Sorry, I misunderstood you. It makes sense to add such comments.

HsiangKai updated this revision to Diff 290859.Sep 9 2020, 6:44 PM

Update according to @rogfer01's comments.

evandro accepted this revision.Sep 29 2020, 1:26 PM

LGTM

This revision is now accepted and ready to land.Sep 29 2020, 1:26 PM