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

[Alex Bradbury via Phabricator via llvm-commits]

asb marked 5 inline comments as done.
asb added inline comments.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:136
+  bool isFenceArg() const {
+    const MCExpr *Val = getImm();
+    auto *SVal = dyn_cast<MCSymbolRefExpr>(Val);
----------------
reames wrote:
> I think you're missing an isImm check here?
It shouldn't be necessary, but yes - let's add it in case things change in the future. Thanks.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:147
+    for (char c : Str) {
+      if (c != 'i' && c != 'o' && c != 'r' && c != 'w')
+        return false;
----------------
reames wrote:
> really minor: a switch would be more clear
Do you find this clearer? It seems slightly less clear to me, but obviously these things are very subjective.

```
    for (char c : Str) {
       if (c <= Prev)
          return false;
      switch (c) {
      default:
        return false;
      case 'i':
      case 'o':
      case 'r':
      case 'w':
        Prev = c;
      }
    }

```


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:149
+        return false;
+      if (c <= Prev)
+        return false;
----------------
reames wrote:
> Should this check be inverted for an ascending order?
'iorw' is accepted, but 'wroi' would not be, matching the GCC behaviour. Reading the *AsmParser.cpp files is made somewhat confusing by the fact methods like ParseInstruction use false for success, unlike these predicates (which are called by tablegenned code).


================
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
----------------
reames wrote:
> Better to invert this and make the error the early return.
I played around with this, and think early-exit for success reads more clearly, particularly as I want to consistently early exit on the same condition (e.g. a couple of lines above we also early-exist on success). There are many more possible incorrect inputs than correct ones, so filtering out the correct ones and having a catch-all for failures at the end makes more sense to me. Happy to change if you feel strongly otherwise.


https://reviews.llvm.org/D23566