[RISCV] Rework .option arch target streamer interface

[jrtc27]

Summary

The current interface requires some rather ugly tracking of state due to
splitting up the calls for each argument. Instead, pack them all into a
single call by passing an ArrayRef. Also clean up the dodgy whitespace
emitted for the directive whilst here; there was a stray space between
the tab and .option, and there was a tab rather than a space after the
first comma for some strange reason.

[jrtc27]

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

2743

NB: This is the reason for using std::string rather than StringRef in RISCVOptionArchArg

[craig.topper]

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

2743

Why do we need to emit a canonical ISA string instead of what was passed?

[craig.topper]

LGTM

[jrtc27]

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

2743

This is a good question, especially when we *don't* include a version number for .option arch, [+-]... dunno what makes more sense, but this was intended to be NFC (as was the dependency, modulo diagnostics)

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") {
-    bool PrefixEmitted = false;
-    bool IsExtensionList = false;
+    SmallVector<RISCVOptionArchArg> Args;
     do {
       if (Parser.parseComma())
         return true;
 
-      bool IsAdd, IsFull;
-      if (parseOptionalToken(AsmToken::Plus)) {
-        IsAdd = true;
-        IsFull = false;
-        IsExtensionList = true;
-      } else if (parseOptionalToken(AsmToken::Minus)) {
-        IsAdd = false;
-        IsFull = false;
-        IsExtensionList = true;
-      } else {
-        if (IsExtensionList)
-          return Error(Parser.getTok().getLoc(),
-                       "unexpected token, expected + or -");
-
-        IsFull = true;
-      }
+      RISCVOptionArchArgType Type;
+      if (parseOptionalToken(AsmToken::Plus))
+        Type = RISCVOptionArchArgType::Plus;
+      else if (parseOptionalToken(AsmToken::Minus))
+        Type = RISCVOptionArchArgType::Minus;
+      else if (!Args.empty())
+      else
+        Type = RISCVOptionArchArgType::Full;
 
       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) {
+      if (Type == RISCVOptionArchArgType::Full) {
         std::string Result;
         if (resetToArch(Arch, Loc, Result, true))
           return true;
 
-        getTargetStreamer().emitDirectiveOptionArchFullArch(Result,
-                                                            PrefixEmitted);
-
+        Args.emplace_back(Type, Result);
         break;
       }
 
       ArrayRef<SubtargetFeatureKV> KVArray(RISCVFeatureKV);
       auto Ext = llvm::lower_bound(KVArray, Arch);
       if (Ext == KVArray.end() || StringRef(Ext->Key) != Arch ||
           !RISCVISAInfo::isSupportedExtension(Arch)) {
         if (isDigit(Arch.back()))
           return Error(
               Loc,
               "Extension version number parsing not currently implemented");
         return Error(Loc, "unknown extension feature");
       }
 
-      bool HasComma = getTok().is(AsmToken::Comma);
-      if (IsAdd) {
+      Args.emplace_back(Type, Ext->Key);
+
+      if (Type == RISCVOptionArchArgType::Plus) {
         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) {
             OutputErrMsg << ErrMsg.getMessage();
           });
 
           return Error(Loc, OutputErrMsg.str());
         }
-        getTargetStreamer().emitDirectiveOptionArchPlusOrMinus(
-            Ext->Key, /*Enable*/ true, PrefixEmitted, HasComma);
       } else {
+        assert(Type == RISCVOptionArchArgType::Minus);
         // It is invalid to disable an extension that there are other enabled
         // extensions depend on it.
         // TODO: Make use of RISCVISAInfo to handle this
         for (auto Feature : KVArray) {
           if (getSTI().hasFeature(Feature.Value) &&
               Feature.Implies.test(Ext->Value))
             return Error(Loc,
                          Twine("Can't disable ") + Ext->Key + " extension, " +
                              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 (Parser.parseEOL())
       return true;
 
+    getTargetStreamer().emitDirectiveOptionArch(Args);
     return false;
   }
 
   if (Option == "rvc") {
     if (Parser.parseEOL())
       return true;
 
     getTargetStreamer().emitDirectiveOptionRVC();

llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h

 #include "RISCV.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 
 namespace llvm {
 
 class formatted_raw_ostream;
 
+enum class RISCVOptionArchArgType {
+  Full,
+  Plus,
+  Minus,
+};
+
+struct RISCVOptionArchArg {
+  RISCVOptionArchArgType Type;
+  std::string Value;
+
+  RISCVOptionArchArg(RISCVOptionArchArgType Type, std::string Value)
+      : Type(Type), Value(Value) {}
+};
+
 class RISCVTargetStreamer : public MCTargetStreamer {
   RISCVABI::ABI TargetABI = RISCVABI::ABI_Unknown;
 
 public:
   RISCVTargetStreamer(MCStreamer &S);
   void finish() override;
   virtual void reset();
 
   virtual void emitDirectiveOptionPush();
   virtual void emitDirectiveOptionPop();
   virtual void emitDirectiveOptionPIC();
   virtual void emitDirectiveOptionNoPIC();
   virtual void emitDirectiveOptionRVC();
   virtual void emitDirectiveOptionNoRVC();
   virtual void emitDirectiveOptionRelax();
   virtual void emitDirectiveOptionNoRelax();
+  virtual void emitDirectiveOptionArch(ArrayRef<RISCVOptionArchArg> Args);
   virtual void emitDirectiveVariantCC(MCSymbol &Symbol);
-  virtual void emitDirectiveOptionArchFullArch(StringRef Value,
-                                               bool &PrefixEmitted);
-  virtual void emitDirectiveOptionArchPlusOrMinus(StringRef Value, bool Enable,
-                                                  bool &PrefixEmitted,
-                                                  bool EmitComma);
   virtual void emitAttribute(unsigned Attribute, unsigned Value);
   virtual void finishAttributeSection();
   virtual void emitTextAttribute(unsigned Attribute, StringRef String);
   virtual void emitIntTextAttribute(unsigned Attribute, unsigned IntValue,
                                     StringRef StringValue);
 
   void emitTargetAttributes(const MCSubtargetInfo &STI, bool EmitStackAlign);
   void setTargetABI(RISCVABI::ABI ABI);
public:
   void emitDirectiveOptionPush() override;
   void emitDirectiveOptionPop() override;
   void emitDirectiveOptionPIC() override;
   void emitDirectiveOptionNoPIC() override;
   void emitDirectiveOptionRVC() override;
   void emitDirectiveOptionNoRVC() override;
   void emitDirectiveOptionRelax() override;
   void emitDirectiveOptionNoRelax() override;
+  void emitDirectiveOptionArch(ArrayRef<RISCVOptionArchArg> Args) override;
   void emitDirectiveVariantCC(MCSymbol &Symbol) override;
-  void emitDirectiveOptionArchFullArch(StringRef Value,
-                                       bool &PrefixEmitted) override;
-  void emitDirectiveOptionArchPlusOrMinus(StringRef Value, bool Enable,
-                                          bool &PrefixEmitted,
-                                          bool EmitComma) override;
 };
 
 }
 #endif

llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp

 void RISCVTargetStreamer::emitDirectiveOptionPush() {}
 void RISCVTargetStreamer::emitDirectiveOptionPop() {}
 void RISCVTargetStreamer::emitDirectiveOptionPIC() {}
 void RISCVTargetStreamer::emitDirectiveOptionNoPIC() {}
 void RISCVTargetStreamer::emitDirectiveOptionRVC() {}
 void RISCVTargetStreamer::emitDirectiveOptionNoRVC() {}
 void RISCVTargetStreamer::emitDirectiveOptionRelax() {}
 void RISCVTargetStreamer::emitDirectiveOptionNoRelax() {}
+void RISCVTargetStreamer::emitDirectiveOptionArch(
+    ArrayRef<RISCVOptionArchArg> Args) {}
 void RISCVTargetStreamer::emitDirectiveVariantCC(MCSymbol &Symbol) {}
-void RISCVTargetStreamer::emitDirectiveOptionArchFullArch(StringRef Value,
-                                                          bool &PrefixEmitted) {
-}
-void RISCVTargetStreamer::emitDirectiveOptionArchPlusOrMinus(
-    StringRef Value, bool Enable, bool &PrefixEmitted, bool EmitComma) {}
 void RISCVTargetStreamer::emitAttribute(unsigned Attribute, unsigned Value) {}
 void RISCVTargetStreamer::finishAttributeSection() {}
 void RISCVTargetStreamer::emitTextAttribute(unsigned Attribute,
                                             StringRef String) {}
 void RISCVTargetStreamer::emitIntTextAttribute(unsigned Attribute,
                                                unsigned IntValue,
                                                StringRef StringValue) {}
 void RISCVTargetStreamer::setTargetABI(RISCVABI::ABI ABI) {

 void RISCVTargetAsmStreamer::emitDirectiveOptionRelax() {
   OS << "\t.option\trelax\n";
 }
 
 void RISCVTargetAsmStreamer::emitDirectiveOptionNoRelax() {
   OS << "\t.option\tnorelax\n";
 }
 
+void RISCVTargetAsmStreamer::emitDirectiveOptionArch(
+    ArrayRef<RISCVOptionArchArg> Args) {
+  OS << "\t.option\tarch";
+  for (const auto &Arg : Args) {
+    OS << ", ";
+    switch (Arg.Type) {
+    case RISCVOptionArchArgType::Full:
+      break;
+    case RISCVOptionArchArgType::Plus:
+      OS << "+";
+      break;
+    case RISCVOptionArchArgType::Minus:
+      OS << "-";
+      break;
+    }
+    OS << Arg.Value;
+  }
+  OS << "\n";
+}
+
 void RISCVTargetAsmStreamer::emitDirectiveVariantCC(MCSymbol &Symbol) {
   OS << "\t.variant_cc\t" << Symbol.getName() << "\n";
 }
 
 void RISCVTargetAsmStreamer::emitAttribute(unsigned Attribute, unsigned Value) {
   OS << "\t.attribute\t" << Attribute << ", " << Twine(Value) << "\n";
 }
 
 void RISCVTargetAsmStreamer::emitTextAttribute(unsigned Attribute,
                                                StringRef String) {
   OS << "\t.attribute\t" << Attribute << ", \"" << String << "\"\n";
 }
 
 void RISCVTargetAsmStreamer::emitIntTextAttribute(unsigned Attribute,
                                                   unsigned IntValue,
                                                   StringRef StringValue) {}
 
-static void emitDirectiveOptionArchPrefix(formatted_raw_ostream &OS,
-                                          bool &PrefixEmitted) {
-  if (!PrefixEmitted) {
-    OS << "\t .option\tarch,\t";
-    PrefixEmitted = true;
-  }
-}
-
-static void emitCommaOrNextLine(formatted_raw_ostream &OS, bool EmitComma) {
-  if (EmitComma)
-    OS << ", ";
-  else
-    OS << "\n";
-}
-
-void RISCVTargetAsmStreamer::emitDirectiveOptionArchFullArch(
-    StringRef Value, bool &PrefixEmitted) {
-  emitDirectiveOptionArchPrefix(OS, PrefixEmitted);
-  OS << Value;
-  emitCommaOrNextLine(OS, false);
-}
-
-void RISCVTargetAsmStreamer::emitDirectiveOptionArchPlusOrMinus(
-    StringRef Value, bool Enable, bool &PrefixEmitted, bool EmitComma) {
-  emitDirectiveOptionArchPrefix(OS, PrefixEmitted);
-  OS << (Enable ? "+" : "-") << Value;
-  emitCommaOrNextLine(OS, EmitComma);
-}
-
 void RISCVTargetAsmStreamer::finishAttributeSection() {}