[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 ↗

(On Diff #374727)

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 ↗

(On Diff #374727)

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");