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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:923
 // 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,
----------------
jrtc27 wrote:
> 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.
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...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110308