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

[jrtc27]

Summary

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.

[craig.topper]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

2745

I think you could use !IsExtensionList here and avoid the IsFull flag. That might make the code less readable, but maybe that can be fixed with renaming IsExtensionList

[craig.topper]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

2745

Nevermind, I see the followup patch changes this to remove IsExtensionList

[craig.topper]

LGTM

[jrtc27]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

2745

I guess I could still as the interim state rather than introduce an unnecessary extra variable, though probably not worth the hassle of changing this revision for code that's going to immediately be deleted

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

if (Option == "pop") {
     getTargetStreamer().emitDirectiveOptionPop();
     if (popFeatureBits())
       return Error(StartLoc, ".option pop with no .option push");
 
     return false;
   }
 
   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(),
 
+      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);
         auto ParseResult = RISCVFeatures::parseFeatureBits(isRV64(), STI->getFeatureBits());
         if (!ParseResult) {
           std::string Buffer;
           raw_string_ostream OutputErrMsg(Buffer);
           handleAllErrors(ParseResult.takeError(), [&](llvm::StringError &ErrMsg) {
do {
                              Feature.Key + " extension requires " + Ext->Key +
                              " extension be enabled");
         }
 
         clearFeatureBits(Ext->Value, Ext->Key);
         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") {
     if (Parser.parseEOL())
       return true;
 
     getTargetStreamer().emitDirectiveOptionRVC();
     setFeatureBits(RISCV::FeatureStdExtC, "c");

llvm/test/MC/RISCV/option-invalid.s

 .option "str"
 
 # CHECK: :[[#@LINE+1]]:13: error: expected newline
 .option rvc foo
 
 # 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
 .option arch, +"c"
 
 # CHECK: :[[#@LINE+1]]:16: error: unknown extension feature
 .option arch, +x
 
 # CHECK: :[[#@LINE+1]]:16: error: unknown extension feature
 .option arch, +relax
 
 # 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
 .option arch, +c2p0
 
 .option arch, +d
 # CHECK: :[[#@LINE+1]]:16: error: Can't disable f extension, d extension requires f extension be enabled
 .option arch, -f