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

[David Chisnall via llvm-commits]

theraven added a comment.

Comments inline.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:162
@@ +161,3 @@
+    return (isConstantImm() && isInt<13>(getConstantImm()) &&
+            getConstantImm() % 2 == 0);
+  }
----------------
This probably wants to be `SImmScaled<12,1>(getConstantImm())`.

It would also be nice to use the same terminology (scaled immediate) as other back ends and the generic code, rather than simm-mask.

================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:171
@@ +170,3 @@
+    return (isConstantImm() && isInt<21>(getConstantImm()) &&
+            getConstantImm() % 2 == 0);
+  }
----------------
As above, should use `SImmScaled`.

================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:262
@@ -227,1 +261,3 @@
 
+bool RISCVAsmParser::generateImmOutOfRangeError(
+    OperandVector &Operands, uint64_t ErrorInfo, int Lower, int Upper,
----------------
If this is only used in this file, it might be better off as a function in an anonymous namespace rather than a method exposed in the header.

================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:314
@@ +313,3 @@
+    return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 20) - 1);
+  case Match_InvalidSImm21Mask1:
+    return generateImmOutOfRangeError(
----------------
I agree with reames that it would be nicer to have the ranges come sensibly from TableGen.  If you figure out a way to do this, let me know as we are currently specifying the same ranges in three different ways for a few things in the CHERI back end...


https://reviews.llvm.org/D23566