[RISCV] Add invalid match case for uimm2, uimm3 and uimm7

[craig.topper]

Can this be tested?

[Jim]

Add testcases and fix incorrect operand of inst alias for InsnR4

[Jim]

Ping?

[craig.topper]

In D110308#3019563, @Jim wrote:

Ping?

Please don't ping unless it's been about a week with no response. An email gets sent to the reviewers on any update and any update moves it to the top of our review queue on the website. So pinging again in the same 24 hours is kind of rude.

[craig.topper]

LGTM

[Jim]

In D110308#3019569, @craig.topper wrote:

In D110308#3019563, @Jim wrote:

Ping?

Please don't ping unless it's been about a week with no response. An email gets sent to the reviewers on any update and any update moves it to the top of our review queue on the website. So pinging again in the same 24 hours is kind of rude.

Sorry about that. I would be careful next time.

[Jim]

Rename funct7 to funct2.

[Jim]

Add missing unrenamed operand.

[jrtc27]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

923

You had a LGTM and now you're breaking thing by uploading new revisions that change this? R-type instructions have a funct7 field, not a funct2 field. I don't understand what you're doing here.

[jrtc27]

llvm/lib/Target/RISCV/RISCVInstrInfo.td

923

Oh, this is the instruction definition for supporting bogus .insn r for R4 formats. Yes this should be uimm2. But I don't like the approach of tacking this onto a patch you already got a LGTM, especially when it's a different fix to what the patch subject says it's doing. At least amend the existing revision and make it clear you're expanding the scope compared with what you first put up for review...

[Jim]

Move the change for incorrect operand type to D110381

[Jim]

I have created new revision D110381 for this.

[craig.topper]

LGTM

[Jim]

Hi @jrtc27,

Does it look good for you? Thanks for your review.

[Jim]

Ping? Thanks.

[craig.topper]

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

1097

Can you put this between UImm5 and SImm5 so that similar code is together. I think that's better than keeping UImm5 and SImm5 next to each other.

[Jim]

Address @craig.topper's comment.

[craig.topper]

LGTM. I don't think we need @jrtc27 to review this.

[jrtc27]

You can have a review anyway :)

[craig.topper]

In D110308#3065962, @jrtc27 wrote:

You can have a review anyway :)

Thanks!

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

bool RISCVAsmParser::MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
   case Match_InvalidUImmLog2XLenNonZero:
     if (isRV64())
       return generateImmOutOfRangeError(Operands, ErrorInfo, 1, (1 << 6) - 1);
     return generateImmOutOfRangeError(Operands, ErrorInfo, 1, (1 << 5) - 1);
   case Match_InvalidUImmLog2XLenHalf:
     if (isRV64())
       return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 5) - 1);
     return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 4) - 1);
+  case Match_InvalidUImm2:
+    return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 2) - 1);
+  case Match_InvalidUImm3:
+    return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 3) - 1);
   case Match_InvalidUImm5:
     return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 5) - 1);
   case Match_InvalidSImm5:
     return generateImmOutOfRangeError(Operands, ErrorInfo, -(1 << 4),
                                       (1 << 4) - 1);
   case Match_InvalidSImm6:
     return generateImmOutOfRangeError(Operands, ErrorInfo, -(1 << 5),
                                       (1 << 5) - 1);
   case Match_InvalidSImm6NonZero:
     return generateImmOutOfRangeError(
         Operands, ErrorInfo, -(1 << 5), (1 << 5) - 1,
         "immediate must be non-zero in the range");
+  case Match_InvalidUImm7:
+    return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 7) - 1);
   case Match_InvalidCLUIImm:
     return generateImmOutOfRangeError(
         Operands, ErrorInfo, 1, (1 << 5) - 1,
         "immediate must be in [0xfffe0, 0xfffff] or");
   case Match_InvalidUImm7Lsb00:
     return generateImmOutOfRangeError(
         Operands, ErrorInfo, 0, (1 << 7) - 4,
         "immediate must be a multiple of 4 bytes in the range");

llvm/lib/Target/RISCV/RISCVInstrInfo.td

 // into a mnemonic to use as the key for the tablegened asm matcher table. The
 // parser will take care of creating these fake mnemonics and will only do it
 // for known formats.
 let EmitPriority = 0 in {
 def : InstAlias<".insn_r $opcode, $funct3, $funct7, $rd, $rs1, $rs2",
                 (InsnR AnyReg:$rd, uimm7:$opcode, uimm3:$funct3, uimm7:$funct7,
                        AnyReg:$rs1, AnyReg:$rs2)>;
 // Accept 4 register form of ".insn r" as alias for ".insn r4".
-def : InstAlias<".insn_r $opcode, $funct3, $funct7, $rd, $rs1, $rs2, $rs3",
-                (InsnR4 AnyReg:$rd, uimm7:$opcode, uimm3:$funct3, uimm7:$funct7,
+def : InstAlias<".insn_r $opcode, $funct3, $funct2, $rd, $rs1, $rs2, $rs3",
+                (InsnR4 AnyReg:$rd, uimm7:$opcode, uimm3:$funct3, uimm2:$funct2,
                         AnyReg:$rs1, AnyReg:$rs2, AnyReg:$rs3)>;
-def : InstAlias<".insn_r4 $opcode, $funct3, $funct7, $rd, $rs1, $rs2, $rs3",
-                (InsnR4 AnyReg:$rd, uimm7:$opcode, uimm3:$funct3, uimm7:$funct7,
+def : InstAlias<".insn_r4 $opcode, $funct3, $funct2, $rd, $rs1, $rs2, $rs3",
+                (InsnR4 AnyReg:$rd, uimm7:$opcode, uimm3:$funct3, uimm2:$funct2,
                         AnyReg:$rs1, AnyReg:$rs2, AnyReg:$rs3)>;
 def : InstAlias<".insn_i $opcode, $funct3, $rd, $rs1, $imm12",
                 (InsnI AnyReg:$rd, uimm7:$opcode, uimm3:$funct3, AnyReg:$rs1,
                        simm12:$imm12)>;
 def : InstAlias<".insn_i $opcode, $funct3, $rd, ${imm12}(${rs1})",
                 (InsnI_Mem AnyReg:$rd, uimm7:$opcode, uimm3:$funct3,
                            AnyReg:$rs1, simm12:$imm12)>;
 def : InstAlias<".insn_b $opcode, $funct3, $rs1, $rs2, $imm12",

llvm/test/MC/RISCV/insn-invalid.s

 # RUN: not llvm-mc -triple riscv32 < %s 2>&1 | FileCheck %s
 
 # Too many operands
 .insn i  0x13,  0,  a0, a1, 13, 14 # CHECK: :[[@LINE]]:33: error: invalid operand for instruction
 .insn r  0x43,  0,  0, fa0, fa1, fa2, fa3, fa4 # CHECK: :[[@LINE]]:44: error: invalid operand for instruction
 
 # Too few operands
 .insn r  0x33,  0,  0, a0, a1 # CHECK: :[[@LINE]]:1: error: too few operands for instruction
 .insn i  0x13,  0,  a0, a1 # CHECK: :[[@LINE]]:1: error: too few operands for instruction
 
 .insn r  0x33,  0,  0, a0, 13 # CHECK: :[[@LINE]]:28: error: invalid operand for instruction
 .insn i  0x13,  0, a0, a1, a2 # CHECK: :[[@LINE]]:28: error: operand must be a symbol with %lo/%pcrel_lo/%tprel_lo modifier or an integer in the range [-2048, 2047]
 
 .insn q  0x13,  0,  a0, a1, 13, 14 # CHECK: :[[@LINE]]:7: error: invalid instruction format
 
+# Invalid immediate
+.insn i  0x99,  0, a0, 4(a1) # CHECK: :[[@LINE]]:10: error: immediate must be an integer in the range [0, 127]
+.insn r  0x33,  8,  0, a0, a1, a2 # CHECK: :[[@LINE]]:17: error: immediate must be an integer in the range [0, 7]
+.insn r4 0x43,  0,  4, fa0, fa1, fa2, fa3 # CHECK: :[[@LINE]]:21: error: immediate must be an integer in the range [0, 3]
+
 # Make fake mnemonics we use to match these in the tablegened asm match table isn't exposed.
 .insn_i  0x13,  0,  a0, a1, 13, 14 # CHECK: :[[@LINE]]:1: error: unknown directive