[PATCH] D23566: [RISCV 8/10] Add support for all RV32I instructions
Philip Reames via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Sep 10 19:18:55 PDT 2017
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
More information about the llvm-commits
mailing list