[RISCV] Add missing part of instruction vmsge {u}. VX

[joker881]

Summary

riscv-v-spec-0.10 page 59

The fourth case of instruction description is added

[craig.topper]

Upload patches with full context using git diff -U99999 as documented here https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

2397

Please wrap this line as clang-format says.

2416

Please indent these comments as the clang-format says.

[HsiangKai]

Do we need this case? Case 4 is any vd. It means (vd == v0) || (vd != v0), right? These two cases are already covered by case 2 and case 3.

[craig.topper]

In D100115#2678649, @HsiangKai wrote:

Do we need this case? Case 4 is any vd. It means (vd == v0) || (vd != v0), right? These two cases are already covered by case 2 and case 3.

I think this is the case that a temp register is provided and the destination isn't v0. The user probably should use case 2 by not providing the temp register. But if they give the temp register should we accept it and match the spec or error for the destination not being v0(what we currently do)?

[joker881]

clang-format the code

[HsiangKai]

In D100115#2678655, @craig.topper wrote:

In D100115#2678649, @HsiangKai wrote:

Do we need this case? Case 4 is any vd. It means (vd == v0) || (vd != v0), right? These two cases are already covered by case 2 and case 3.

I think this is the case that a temp register is provided and the destination isn't v0. The user probably should use case 2 by not providing the temp register. But if they give the temp register should we accept it and match the spec or error for the destination not being v0(what we currently do)?

If the destination is not v0, case 2 is better than case 4. There should be no need to provide the temp register. I think the case 4 is useful when we could not determine vd is v0 or not, but it should not be the case in AsmParser. The registers in MCInst should be physical registers.

[joker881]

In D100115#2677432, @craig.topper wrote:

Upload patches with full context using git diff -U99999 as documented here https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Ok, I have finished it. B97933: Diff 336392.

[craig.topper]

In D100115#2678857, @HsiangKai wrote:

In D100115#2678655, @craig.topper wrote:

In D100115#2678649, @HsiangKai wrote:

Do we need this case? Case 4 is any vd. It means (vd == v0) || (vd != v0), right? These two cases are already covered by case 2 and case 3.

I think this is the case that a temp register is provided and the destination isn't v0. The user probably should use case 2 by not providing the temp register. But if they give the temp register should we accept it and match the spec or error for the destination not being v0(what we currently do)?

If the destination is not v0, case 2 is better than case 4. There should be no need to provide the temp register. I think the case 4 is useful when we could not determine vd is v0 or not, but it should not be the case in AsmParser. The registers in MCInst should be physical registers.

What does the GNU assembler do?

Can someone write inline assembly that requires this? If they let the compiler allocate the registers for the inline assembly we would need to support any register.

[HsiangKai]

In D100115#2680063, @craig.topper wrote:

In D100115#2678857, @HsiangKai wrote:

In D100115#2678655, @craig.topper wrote:

In D100115#2678649, @HsiangKai wrote:

Do we need this case? Case 4 is any vd. It means (vd == v0) || (vd != v0), right? These two cases are already covered by case 2 and case 3.

I think this is the case that a temp register is provided and the destination isn't v0. The user probably should use case 2 by not providing the temp register. But if they give the temp register should we accept it and match the spec or error for the destination not being v0(what we currently do)?

If the destination is not v0, case 2 is better than case 4. There should be no need to provide the temp register. I think the case 4 is useful when we could not determine vd is v0 or not, but it should not be the case in AsmParser. The registers in MCInst should be physical registers.

What does the GNU assembler do?

Can someone write inline assembly that requires this? If they let the compiler allocate the registers for the inline assembly we would need to support any register.

Got it. Maybe we need this pattern for inline asm. I have no strong opinion to oppose this patch. Just curious when we need it.

[craig.topper]

LGTM

[MaskRay]

Your commit description was not formatted correctly. You can read other commits for references.
Please use proper author name and email instead of root <calico>.

Differential Revision: was not on a new line so the differential was not automatically closed when you pushed the commit.

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

emitToStreamer(Out, MCInstBuilder(Opcode)
                             .addOperand(Inst.getOperand(0))
                             .addOperand(Inst.getOperand(1))
                             .addOperand(Inst.getOperand(2))
                             .addOperand(Inst.getOperand(3)));
     emitToStreamer(Out, MCInstBuilder(RISCV::VMXOR_MM)
                             .addOperand(Inst.getOperand(0))
                             .addOperand(Inst.getOperand(0))
                             .addReg(RISCV::V0));
-  } else if (Inst.getNumOperands() == 5) {
+  } else if (Inst.getNumOperands() == 5 &&
+             Inst.getOperand(0).getReg() == RISCV::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
     assert(Inst.getOperand(0).getReg() == RISCV::V0 &&
            "The destination register should be V0.");
     assert(Inst.getOperand(1).getReg() != RISCV::V0 &&
            "The temporary vector register should not be V0.");
     emitToStreamer(Out, MCInstBuilder(Opcode)
                             .addOperand(Inst.getOperand(1))
                             .addOperand(Inst.getOperand(2))
                             .addOperand(Inst.getOperand(3))
                             .addOperand(Inst.getOperand(4)));
     emitToStreamer(Out, MCInstBuilder(RISCV::VMANDNOT_MM)
                             .addOperand(Inst.getOperand(0))
                             .addOperand(Inst.getOperand(0))
                             .addOperand(Inst.getOperand(1)));
+  } else if (Inst.getNumOperands() == 5) {
+    // masked va >= x, any vd
+    //
+    // pseudoinstruction: vmsge{u}.vx vd, va, x, v0.t, vt
+    // expansion: vmslt{u}.vx vt, va, x; vmandnot.mm vt, v0, vt; vmandnot.mm vd,
+    // vd, v0; vmor.mm vd, vt, vd
+    emitToStreamer(Out, MCInstBuilder(Opcode)
+                            .addOperand(Inst.getOperand(1))
+                            .addOperand(Inst.getOperand(2))
+                            .addOperand(Inst.getOperand(3))
+                            .addReg(RISCV::NoRegister));
+    emitToStreamer(Out, MCInstBuilder(RISCV::VMANDNOT_MM)
+                            .addOperand(Inst.getOperand(1))
+                            .addReg(RISCV::V0)
+                            .addOperand(Inst.getOperand(1)));
+    emitToStreamer(Out, MCInstBuilder(RISCV::VMANDNOT_MM)
+                            .addOperand(Inst.getOperand(0))
+                            .addOperand(Inst.getOperand(0))
+                            .addReg(RISCV::V0));
+    emitToStreamer(Out, MCInstBuilder(RISCV::VMOR_MM)
+                            .addOperand(Inst.getOperand(0))
+                            .addOperand(Inst.getOperand(1))
+                            .addOperand(Inst.getOperand(0)));
   }
 }
 
 bool RISCVAsmParser::checkPseudoAddTPRel(MCInst &Inst,
                                          OperandVector &Operands) {
   assert(Inst.getOpcode() == RISCV::PseudoAddTPRel && "Invalid instruction");
   assert(Inst.getOperand(2).isReg() && "Unexpected second operand kind");
   if (Inst.getOperand(2).getReg() != RISCV::X4) {

llvm/lib/Target/RISCV/RISCVInstrInfoV.td

def PseudoVMSGE_VX : Pseudo<(outs VR:$vd),
                             (ins VR:$vs2, GPR:$rs1),
                             [], "vmsge.vx", "$vd, $vs2, $rs1">;
 def PseudoVMSGEU_VX_M : Pseudo<(outs VRNoV0:$vd),
                                (ins VR:$vs2, GPR:$rs1, VMaskOp:$vm),
                                [], "vmsgeu.vx", "$vd, $vs2, $rs1$vm">;
 def PseudoVMSGE_VX_M : Pseudo<(outs VRNoV0:$vd),
                               (ins VR:$vs2, GPR:$rs1, VMaskOp:$vm),
                               [], "vmsge.vx", "$vd, $vs2, $rs1$vm">;
-def PseudoVMSGEU_VX_M_T : Pseudo<(outs VMV0:$vd, VR:$scratch),
+def PseudoVMSGEU_VX_M_T : Pseudo<(outs VR:$vd, VR:$scratch),
                                  (ins VR:$vs2, GPR:$rs1, VMaskOp:$vm),
                                  [], "vmsgeu.vx", "$vd, $vs2, $rs1$vm, $scratch">;
-def PseudoVMSGE_VX_M_T : Pseudo<(outs VMV0:$vd, VR:$scratch),
+def PseudoVMSGE_VX_M_T : Pseudo<(outs VR:$vd, VR:$scratch),
                                 (ins VR:$vs2, GPR:$rs1, VMaskOp:$vm),
                                 [], "vmsge.vx", "$vd, $vs2, $rs1$vm, $scratch">;
 }
 
 // Vector Integer Min/Max Instructions
 defm VMINU_V : VALU_IV_V_X<"vminu", 0b000100>;
 defm VMIN_V : VALU_IV_V_X<"vmin", 0b000101>;
 defm VMAXU_V : VALU_IV_V_X<"vmaxu", 0b000110>;

llvm/test/MC/RISCV/rvv/compare.s

 vmsge.vx v0, v4, a0, v0.t, v2
 # CHECK-INST: vmslt.vx v2, v4, a0, v0.t
 # CHECK-INST: vmandnot.mm v0, v0, v2
 # CHECK-ENCODING: [0x57,0x41,0x45,0x6c]
 # CHECK-ENCODING: [0x57,0x20,0x01,0x62]
 # CHECK-ERROR: instruction requires the following: 'V' (Vector Instructions)
 # CHECK-UNKNOWN: 57 41 45 6c <unknown>
 # CHECK-UNKNOWN: 57 20 01 62 <unknown>
+
+vmsgeu.vx v9, v4, a0, v0.t, v2
+# CHECK-INST: vmsltu.vx v2, v4, a0
+# CHECK-INST: vmandnot.mm v2, v0, v2
+# CHECK-INST: vmandnot.mm v9, v9, v0
+# CHECK-INST: vmor.mm v9, v2, v9
+# CHECK-ENCODING: [0x57,0x41,0x45,0x6a]
+# CHECK-ENCODING: [0x57,0x21,0x01,0x62]
+# CHECK-ENCODING: [0xd7,0x24,0x90,0x62]
+# CHECK-ENCODING: [0xd7,0xa4,0x24,0x6a]
+# CHECK-ERROR: instruction requires the following: 'V' (Vector Instructions)
+# CHECK-UNKNOWN: 57 41 45 6a <unknown>
+# CHECK-UNKNOWN: 57 21 01 62 <unknown>
+# CHECK-UNKNOWN: d7 24 90 62 <unknown>
+# CHECK-UNKNOWN: d7 a4 24 6a <unknown>
+
+vmsge.vx v8, v4, a0, v0.t, v2
+# CHECK-INST: vmslt.vx v2, v4, a0
+# CHECK-INST: vmandnot.mm v2, v0, v2
+# CHECK-INST: vmandnot.mm v8, v8, v0
+# CHECK-INST: vmor.mm v8, v2, v8
+# CHECK-ENCODING: [0x57,0x41,0x45,0x6e]
+# CHECK-ENCODING: [0x57,0x21,0x01,0x62]
+# CHECK-ENCODING: [0x57,0x24,0x80,0x62]
+# CHECK-ENCODING: [0x57,0x24,0x24,0x6a]
+# CHECK-ERROR: instruction requires the following: 'V' (Vector Instructions)
+# CHECK-UNKNOWN: 57 41 45 6e <unknown>
+# CHECK-UNKNOWN: 57 21 01 62 <unknown>
+# CHECK-UNKNOWN: 57 24 80 62 <unknown>
+# CHECK-UNKNOWN: 57 24 24 6a <unknown>
 No newline at end of file

llvm/test/MC/RISCV/rvv/invalid.s

 vadd.vi v0, v2, 1, v0.t
 # CHECK-ERROR: The destination vector register group cannot overlap the mask register.
 # CHECK-ERROR-LABEL: vadd.vi v0, v2, 1, v0.t
 
 vmsge.vx v0, v4, a0, v0.t
 # CHECK-ERROR: too few operands for instruction
 # CHECK-ERROR-LABEL: vmsge.vx v0, v4, a0, v0.t
 
-vmsge.vx v8, v4, a0, v0.t, v2
-# CHECK-ERROR: invalid operand for instruction
-# CHECK-ERROR-LABEL: vmsge.vx v8, v4, a0, v0.t, v2
-
 vmerge.vim v0, v1, 1, v0
 # CHECK-ERROR: The destination vector register group cannot be V0.
 # CHECK-ERROR-LABEL: vmerge.vim v0, v1, 1, v0
 
 vmerge.vvm v0, v1, v2, v0
 # CHECK-ERROR: The destination vector register group cannot be V0.
 # CHECK-ERROR-LABEL: vmerge.vvm v0, v1, v2, v0