[PATCH] D23566: [RISCV 8/10] Add support for all RV32I instructions

[Philip Reames via llvm-commits]

reames added a subscriber: reames.

================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:139
@@ -135,3 +138,3 @@
 
   bool issimm12() const {
     return (isConstantImm() && isInt<12>(getConstantImm()));
----------------
style: shouldn't this be isImm12?

================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:143
@@ -139,1 +142,3 @@
 
+  bool isimm20() const {
+    return (isConstantImm() && isUInt<20>(getConstantImm()));
----------------
Style: sort these by immediate size.

================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:148
@@ +147,3 @@
+  bool isimm4() const {
+    return (isConstantImm() && isUInt<4>(getConstantImm()));
+  }
----------------
style: duplicated code, possible an isImm<X>?  With wrappers potentially?

================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:160
@@ +159,3 @@
+
+  bool issimm13maskb0() const {
+    return (isConstantImm() && isInt<13>(getConstantImm()) &&
----------------
I saw this being discussed in a previous review, but I won't know what these were from the names.  Possible a comment?  Or a pointer to a design decision?

================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:297
@@ +296,3 @@
+    return Error(ErrorLoc,
+                 "immediate must be an integer in the range [0, 1048575]");
+  case Match_Invalidsimm21maskb0:
----------------
Style: Hard coding these values seems slightly error prone.  Could we generate these messages from the immediate size and common all of this code?

================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:109
@@ +108,3 @@
+// TODO: how to handle difference in the instruction for RV32I and RV64I.
+// Should take an imm6 shamt and check in MatchAndEmitInstruction
+
----------------
Shouldn't this simply be two different instructions with disambiguation living in the disassembler?


https://reviews.llvm.org/D23566