[llvm] 2c11768 - [RISCV] Make .option arch parser less mind-bending

[Jessica Clarke via llvm-commits]

Author: Jessica Clarke
Date: 2023-06-06T14:56:20+01:00
New Revision: 2c11768eee3d9301eca52e8756e4088b45108446

URL: https://github.com/llvm/llvm-project/commit/2c11768eee3d9301eca52e8756e4088b45108446
DIFF: https://github.com/llvm/llvm-project/commit/2c11768eee3d9301eca52e8756e4088b45108446.diff

LOG: [RISCV] Make .option arch parser less mind-bending

Currently the early-return flow in the infinite loop makes it hard to
find the non-error termination points amongst the sea of errors. Rewrite
it with a more conventional control flow that has a clear loop guard (in
place of one of the early returns) and a break (in place of the other),
and with greater code reuse.

This has a small effect on the errors given for malformed input, as seen
in the affected test, and is probably more helpful as a result. Note
that we also bail early now if parseComma fails, as is standard.

Reviewed By: craig.topper

Differential Revision: https://reviews.llvm.org/D152192

Added: 
    

Modified: 
    llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
    llvm/test/MC/RISCV/option-invalid.s

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index f7af251d838b4..9e2e884c17ba6 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -2711,67 +2711,59 @@ bool RISCVAsmParser::parseDirectiveOption() {
   }
 
   if (Option == "arch") {
-
-    Parser.parseComma();
-
     bool PrefixEmitted = false;
     bool IsExtensionList = false;
-    while (true) {
-      bool IsAdd;
-      if (Parser.getTok().is(AsmToken::Plus)) {
+    do {
+      if (Parser.parseComma())
+        return true;
+
+      bool IsAdd, IsFull;
+      if (parseOptionalToken(AsmToken::Plus)) {
         IsAdd = true;
+        IsFull = false;
         IsExtensionList = true;
-      } else if (Parser.getTok().is(AsmToken::Minus)) {
+      } else if (parseOptionalToken(AsmToken::Minus)) {
         IsAdd = false;
+        IsFull = false;
         IsExtensionList = true;
       } else {
-        SMLoc ArchLoc = Parser.getTok().getLoc();
-
         if (IsExtensionList)
-          return Error(ArchLoc, "unexpected token, expected + or -");
+          return Error(Parser.getTok().getLoc(),
+                       "unexpected token, expected + or -");
 
-        StringRef Arch;
-        if (Parser.getTok().is(AsmToken::Identifier))
-          Arch = Parser.getTok().getString();
-        else
-          return Error(ArchLoc,
-                       "unexpected token, expected identifier");
+        IsFull = true;
+      }
 
+      if (Parser.getTok().isNot(AsmToken::Identifier))
+        return Error(Parser.getTok().getLoc(),
+                     "unexpected token, expected identifier");
+
+      StringRef Arch = Parser.getTok().getString();
+      SMLoc Loc = Parser.getTok().getLoc();
+      Parser.Lex();
+
+      if (IsFull) {
         std::string Result;
-        if (resetToArch(Arch, ArchLoc, Result, true))
+        if (resetToArch(Arch, Loc, Result, true))
           return true;
 
         getTargetStreamer().emitDirectiveOptionArchFullArch(Result,
                                                             PrefixEmitted);
 
-        Parser.Lex();
-
-        return Parser.parseToken(AsmToken::EndOfStatement,
-                                 "unexpected token, expected end of statement");
+        break;
       }
 
-      Parser.Lex();
-
-      if (Parser.getTok().isNot(AsmToken::Identifier))
-        return Error(Parser.getTok().getLoc(),
-                     "unexpected token, expected identifier");
-
-      StringRef ExtStr = Parser.getTok().getString();
-
       ArrayRef<SubtargetFeatureKV> KVArray(RISCVFeatureKV);
-      auto Ext = llvm::lower_bound(KVArray, ExtStr);
-      if (Ext == KVArray.end() || StringRef(Ext->Key) != ExtStr ||
-          !RISCVISAInfo::isSupportedExtension(ExtStr)) {
-        if (isDigit(ExtStr.back()))
+      auto Ext = llvm::lower_bound(KVArray, Arch);
+      if (Ext == KVArray.end() || StringRef(Ext->Key) != Arch ||
+          !RISCVISAInfo::isSupportedExtension(Arch)) {
+        if (isDigit(Arch.back()))
           return Error(
-              Parser.getTok().getLoc(),
+              Loc,
               "Extension version number parsing not currently implemented");
-        return Error(Parser.getTok().getLoc(), "unknown extension feature");
+        return Error(Loc, "unknown extension feature");
       }
 
-      SMLoc Loc = Parser.getTok().getLoc();
-
-      Parser.Lex(); // Eat arch string
       bool HasComma = getTok().is(AsmToken::Comma);
       if (IsAdd) {
         setFeatureBits(Ext->Value, Ext->Key);
@@ -2804,13 +2796,12 @@ bool RISCVAsmParser::parseDirectiveOption() {
         getTargetStreamer().emitDirectiveOptionArchPlusOrMinus(
             Ext->Key, /*Enable*/ false, PrefixEmitted, HasComma);
       }
+    } while (Parser.getTok().isNot(AsmToken::EndOfStatement));
 
-      if (!HasComma)
-        return Parser.parseToken(AsmToken::EndOfStatement,
-                                 "unexpected token, expected end of statement");
-      // Eat comma
-      Parser.Lex();
-    }
+    if (Parser.parseEOL())
+      return true;
+
+    return false;
   }
 
   if (Option == "rvc") {

diff  --git a/llvm/test/MC/RISCV/option-invalid.s b/llvm/test/MC/RISCV/option-invalid.s
index 237fcd961ee81..34d6adabbb093 100644
--- a/llvm/test/MC/RISCV/option-invalid.s
+++ b/llvm/test/MC/RISCV/option-invalid.s
@@ -16,7 +16,7 @@
 # CHECK: :[[#@LINE+1]]:23: error: unexpected token, expected + or -
 .option arch, +f, +d, rv32ifd, -d
 
-# CHECK: :[[#@LINE+1]]:22: error: unexpected token, expected end of statement
+# CHECK: :[[#@LINE+1]]:22: error: expected newline
 .option arch, rv32ifd, +f, +d
 
 # CHECK: :[[#@LINE+1]]:16: error: unexpected token, expected identifier
@@ -31,7 +31,7 @@
 # CHECK: :[[#@LINE+1]]:16: error: unexpected token, expected identifier
 .option arch, +
 
-# CHECK: :[[#@LINE+1]]:18: error: unexpected token, expected end of statement
+# CHECK: :[[#@LINE+1]]:18: error: expected comma
 .option arch, +c foo
 
 # CHECK: :[[#@LINE+1]]:16: error: Extension version number parsing not currently implemented