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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

LGTM w/comments applied before commit.

Note: I'm LGTM this after looking for mostly stylistic issues.  I did not closely review the ISA specification to confirm the RISCV specific instruction details.  I'm mostly LGTMing this because it's been stuck in review for a while, I want to get it unblocked, and I don't see any obvious reasons to hold it back.



================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:135
+
+  bool isFenceArg() const {
+    const MCExpr *Val = getImm();
----------------
/// comment describing function


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:136
+  bool isFenceArg() const {
+    const MCExpr *Val = getImm();
+    auto *SVal = dyn_cast<MCSymbolRefExpr>(Val);
----------------
I think you're missing an isImm check here?


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:147
+    for (char c : Str) {
+      if (c != 'i' && c != 'o' && c != 'r' && c != 'w')
+        return false;
----------------
really minor: a switch would be more clear


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:149
+        return false;
+      if (c <= Prev)
+        return false;
----------------
Should this check be inverted for an ascending order?


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:262
+    assert(N == 1 && "Invalid number of operands!");
+    auto SE = dyn_cast<MCSymbolRefExpr>(getImm());
+    assert(SE && "FenceArg should have been validated by isFenceArg");
----------------
Just use a cast<> and drop the separate assert.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:451
   // Attempt to parse token as an immediate
-  if (parseImmediate(Operands) == MatchOperand_Success)
+  if (parseImmediate(Operands) == MatchOperand_Success) {
+    // Parse memory base register if present
----------------
Better to invert this and make the error the early return.


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:26
+  Pseudo = 0,
+  FrmR = 1,
+  FrmI = 2,
----------------
Is there a need to be particularly short here?  If not, something like InstFormatR might be more clear.  


https://reviews.llvm.org/D23566