[RISCV] Support '.option arch' directive

[StephenFan]

Summary

The proposal of '.option arch' directive is https://github.com/riscv-non-isa/riscv-asm-manual/pull/67

Note: For '.option arch, +/-' directive, version number is not yet supported.

[asb]

Thanks for the patch! One quick suggestion from a rapid scan is that it would be good to have test coverage for illegal extension combinations (including for .option arch removing an extension that is a dependency of another enaabled extension).

[luismarques]

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

2712–2714

Don't we need to check for errors here?

2751–2752

Past reviews mentioned that the plan going forward is for extensions to be immutable and not rely on version numbers. Unless that has changed again, we can delete this comment.

[StephenFan]

1. Use llvm::lower_bound to find corresponding featureKV
2. Check dependency when disable extensions
3. Delete TODO for support version number

[StephenFan]

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

2712–2714

parseComma would raise an error if the token is not the comma. Does this make you happy?

[jrtc27]

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

2716

binutils dropped the leading = but the spec proposal still has it

2781

binutils allows more than one change in the same line as .option arch, +foo, bar, -baz, +qwerty etc

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

32

Allowing this would be more useful IMO, otherwise you need to know the full list of extensions you might need to disable in order to disable another extension, which can vary with the toolchain.

However, binutils takes an alternative approach at the moment and re-computes the implied extensions at the end, so this becomes a no-op as rv32id gets turned back into rv32ifd. I don't think that's particularly helpful behaviour though, I would even call it surprising.

[StephenFan]

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

2781

what would .option arch, bar do?

[jrtc27]

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

2781

That's the replacement syntax for .option arch, =bar, it was changed before merging to binutils

[StephenFan]

1. Drop the leading =
2. Support .option arch, +foo, bar, -baz, +qwerty

[StephenFan]

Ping.

[kito-cheng]

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

95

# Test '.option arch, <arch-string>' directive

98

Spec and binutils implementation are both string here, so it should be .option arch, "rv32i2p0_m2p0_a2p0_c2p0".

101

It's not allowed mixed extension list with arch string.

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

14

.option arch, rv32i should be check invalid here rather than .option arch, "rv32i"

67

nit: plz drop the extra newline.

[luismarques]

@StephenFan Can you please rebase this?

[StephenFan]

In D123515#4158758, @luismarques wrote:

@StephenFan Can you please rebase this?

Yes, I will rebase this these days.

[StephenFan]

Rebase and address comments

[StephenFan]

Change base commit.

[jrtc27]

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

98

It being a string isn't what the proposed spec says, and doesn't make sense to me; +c but "rv64ic" is inconsistent and pointless. If binutils requires it to be a string then fix binutils to not be weird and instead follow the more sensible spec.

[jrtc27]

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

98

But indeed binutils follows the spec and has .option arch, rv64ic not .option arch, "rv64ic". It even tests this in gas/testsuite/gas/riscv/option-arch-03.s. So the implementation here needs fixing to match binutils and the spec.

[StephenFan]

Match binutils and spec.

[luismarques]

You now accept .option arch, rv32ic (without the quotes) but that's not covered in the valid tests.
Also, you're normalizing that to the quoted version .option arch, "rv32i2p0_c2p0" and accepting that but, at first glance, I'm not seeing that as an accepted case in the binutils tests.

The test coverage could be extended a bit to cover e.g.:

• Test accepted cases with comma-separated lists (e.g. +c, +m)
• Test invalid attempt to remove i base ISA
• Test +c, -c (and vice-versa), see if it does the correct thing
• Test multi-letter extensions like +zba
• Test full ISA string, with and without versions

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

214–216

Two nits:
"that expanded" -> "that is expanded"?
Missing period.

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

95

Nit: "stringg"

97

You're still accepting quoted ISA strings and not testing unquoted ISA strings.

[StephenFan]

Address comments.

[jrtc27]

What happened to your new test?

[jrtc27]

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

2732

These errors all need their grammar fixing

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

37

This is a *weird* interface

148

You're still putting quotes around it

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

17

Stop using quotes except when specifically testing that quotes get rejected

[hiraditya]

This patch is needed for zbb support as per: https://lore.kernel.org/all/20230117122447.y6tdsmsxqdwf76ri@orel/

[jrtc27]

In D123515#4189301, @hiraditya wrote:

This patch is needed for zbb support as per: https://lore.kernel.org/all/20230117122447.y6tdsmsxqdwf76ri@orel/

For Zbb support *in the Linux kernel for its hand-written assembly routines*. Zbb works in Clang/LLVM today, and you can get it with -march=...zbb..., whether for compiler-generated code or hand-written assembly.

[hiraditya]

ping @StephenFan

[StephenFan]

Address comments and add back test file.

[StephenFan]

Grammar fixing.

[luismarques]

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

65

Nit: separated

109

You should also test that something like +d, -d enables the dependent extensions (f).

[luismarques]

Overall LGTM. Only some minor points left to address.

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

2711

Nit: unneeded empty line space.

[luismarques]

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

31–32

The error should be that we don't (yet) support version numbers here, not that "extensions don't rely on version numbers", since the spec for .option arch does include explicit support for version numbers. Or just add such support to this patch, instead of being a follow-up patch.

[asb]

First and foremost, thank you for keeping this up to date and iterating upon it while the .option arch proposal was under review in the asm manual. I think we're basically almost there.

I've left a few inline comments (and +1 on Luis' suggestion to cover the case where you e.g. +d then -d, and +f should remain enabled.)

What is your intended behaviour for experimental extensions? It looks like they would be accepted with a fullarch string including the full version, but it seems are rejected for for +foo/-foo (when invoking llvm-mc directly at least). I think that is fine, as we don't really seek to make experimental extensions fully first class citizens. But it seems as though this implementation rejects version numbers in for +foo and -foo while the spec accepts a version such as 2p1 or 2. Accepting a version would give an easy way of supporting experimental extensions too. We should really have some test coverage for experimental extensions.

I _think_ we've probably resolved our discussions on versioning and extensions enough that we shouldn't have too much uncertainty on how this is handled. But once the review comments are addressed, I'd be open to landing this without the version parsing support if you don't have time to look at that - if so, I'd suggest changing the error from "Don't specify version number, extensions don't rely on version numbers" to "Extension version number parsing not currently implemented" or similar.

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

144

Maybe emitDirectiveOptionArchFullArch or similar? As = isn't part of the grammar in the committed version of the spec.

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

17

It would be good to check .option arch rv32ifd, +f, +d or similar (i.e. an input with a fullarch string coming first).

[luismarques]

Adding a test showing that using .option arch in inline assembly doesn't affect ELF attributes nor set the EF_RISCV_RVC ELF flag would be nice. The latter can also be tested for assembly files.

[MaskRay]

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

2728

unneeded line wrapping? Did you use git clang-format on the modified lines?

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

94

here and throughout, add a space in #CHECK:

[StephenFan]

Address comments.

[luismarques]

Thanks for all of the improvements so far.

Two additional problems I noticed:

1. It currently accepts non-extension features like .option arch, +svnapot, +relax.

2. This patch currently allows switching between rv32 and rv64 when using the full arch string. The spec doesn't seem to say anything about that and I could see some obscure uses for it, but I'm not sure we actually want to support it, so it's probably safer to reject it (opinions from others about this are welcome). Either way, we should have test coverage for that scenario.

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

34–39

Nit: don't capitalize the error message, for consistency.

[hiraditya]

ping: @StephenFan

[StephenFan]

Reject +relax

[StephenFan]

Reject switching between rv32 and rv64

[StephenFan]

Reject switching between rv32 and rv64

[StephenFan]

In D123515#4248222, @luismarques wrote:

Thanks for all of the improvements so far.

Two additional problems I noticed:

1. It currently accepts non-extension features like .option arch, +svnapot, +relax.

svnapot can be parsed from arch string. I think we need an API in RISCVISAInfo to decide that some extensions are non-extension features, only rejecting in the logic of parse .option arch directive seems tricky.

[jrtc27]

There's nothing wrong with .option arch, +svnapot; it's an extension, so it's valid to put in the arch string, it's just a bit pointless as it doesn't actually do anything to the toolchain.

[craig.topper]

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

2758

Extensions can have numbers in them that aren't versions. For example "zvl32b". Digits are only version numbers if they are the end of the string.

2777

"It is invalid to disable an extension if there are other enabled extensions that depend on it."

2780

I don't think the implies list is normalized to include transitive implications. For example if A implies B and B implies C. I don't think the C is A's implies list.

This also doesn't cover cases that can't be expressed in the .td file. At one point Zve32f required either F or Zfinx. But F and Zfinx are mutually exclusive so we could't have Zve32f imply either of them in the .td file. All we could do is check it in RISCVISAInfo::checkDependency.

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

150

Can emitDirectiveOptionArchPlus/emitDirectiveOptionArchMinus be combined by passing a bool for + or -?

[hiraditya]

ping: @StephenFan

[StephenFan]

Looks like some users are looking forward to this patch, would you mind accepting it first and then addressing your comments separately @craig.topper ?

[craig.topper]

In D123515#4351899, @StephenFan wrote:

Looks like some users are looking forward to this patch, would you mind accepting it first and then addressing your comments separately @craig.topper ?

Sure. LGTM

[jrtc27]

Issues need fixing before it lands, making exceptions sets a bad precedent

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

114

This seems like a holdover from back when this was allowing quoted strings, and doesn't distinguish it from the test earlier in the file

[StephenFan]

Accept extension name that has digits.

[StephenFan]

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

2780

I don't think the implies list is normalized to include transitive implications. For example if A implies B and B implies C. I don't think the C is A's implies list.

Can this be handled by RISCVISAInfo? When I parse RISCVIAInfo from featureBits, it always updates implication.

[kito-cheng]

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

2780

Yeah, use RISCVISAInfo should be reasonable, no reason to duplicate and maintain the logic here :)

An idea thought here:

• Convert feature bits to feature string vector
• Convert extension list to a vector<string>
• Implement a new parse function static llvm::Expected< std::unique_ptr< RISCVISAInfo > > RISCVISAInfo::parseArchOption (const std::vector< StringRef > &BaseFeatures, const std::vector< StringRef > &ExtensionList), then handle all ISA stuff logic here.

[StephenFan]

Add test that check impliest transtively

[StephenFan]

Add TODO.

[StephenFan]

Check dependency when enabling an extension.

[StephenFan]

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

2780

I don't think the implies list is normalized to include transitive implications. For example if A implies B and B implies C. I don't think the C is A's implies list.

Thanks, @craig.topper and @kito-cheng . But it does check for transitivity here. And I added a test of this:

.option arch, +d, -zicsr

llvm-mc can report an error correctly. So there is no functional mistake and if we want to use RISCVISAInfo, we just need a NFC patch.

This also doesn't cover cases that can't be expressed in the .td file. At one point Zve32f required either F or Zfinx. But F and Zfinx are mutually exclusive so we could't have Zve32f imply either of them in the .td file. All we could do is check it in RISCVISAInfo::checkDependency.

Do you mean like that we can't enable the zfinx extension if f extension exists? If so, I have addressed it.

[craig.topper]

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

2780

I don't think the implies list is normalized to include transitive implications. For example if A implies B and B implies C. I don't think the C is A's implies list.

Thanks, @craig.topper and @kito-cheng . But it does check for transitivity here. And I added a test of this:

.option arch, +d, -zicsr

llvm-mc can report an error correctly. So there is no functional mistake and if we want to use RISCVISAInfo, we just need a NFC patch.

I guess it works because F was implied by D so the check was applied directly against F when Zicsr was disabled.

This also doesn't cover cases that can't be expressed in the .td file. At one point Zve32f required either F or Zfinx. But F and Zfinx are mutually exclusive so we could't have Zve32f imply either of them in the .td file. All we could do is check it in RISCVISAInfo::checkDependency.

Do you mean like that we can't enable the zfinx extension if f extension exists? If so, I have addressed it.

I wasn't specifically referring to that. I was only saying that the implies list in the .td file can be incomplete.

[craig.topper]

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

2780

The .td file for Zve32f requiring F was only updated recently here https://reviews.llvm.org/D150021

[StephenFan]

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

2780

I don't think the implies list is normalized to include transitive implications. For example if A implies B and B implies C. I don't think the C is A's implies list.

Thanks, @craig.topper and @kito-cheng . But it does check for transitivity here. And I added a test of this:

.option arch, +d, -zicsr

llvm-mc can report an error correctly. So there is no functional mistake and if we want to use RISCVISAInfo, we just need a NFC patch.

I guess it works because F was implied by D so the check was applied directly against F when Zicsr was disabled.

Yeah, it works because F is implied when D is enabled. And when we were checking zicsr, F's imply list was checked. So llvm-mc would report an error.
Maybe I have not got your point about " the implies list in the .td file can be incomplete.". Would you mind giving me an assembly example to show my mistake?

[craig.topper]

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

2780

I don't have an example right now. All I'm trying to say is that the Implies field in FeatureKV has been incorrect in the past. 3 weeks ago it did not say that Zve32f implies F. So I think your patch would have allowed F to be disabled when Zve32f was enabled. This is no longer an issue, but I can't guarantee it won't be an issue again in the future.

[craig.topper]

@jrtc27 are there any remaining issues you'd like to see addressed in the first commit. Android developers are really anxious to get this in to enable V extension support in the kernel.

[jrtc27]

In D123515#4370386, @craig.topper wrote:

@jrtc27 are there any remaining issues you'd like to see addressed in the first commit. Android developers are really anxious to get this in to enable V extension support in the kernel.

I still really don't like the target streamer interface... using a bool for +/- would at least help, but IMO this should surely be something like an emitOptionArch that takes a list of string-enum pairs, where the enum is something like Full/Plus/Minus. It's the PrefixEmitted that feels particularly egregious.

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

150

Is clang-format really happy with the lack of blank lines?

[asb]

Am I right in thinking we're at the point now where we're discussing slightly cleaner implementation approaches but are happy the functionality is correct and faithful to the specification? I think a careful review of the functionality is warranted as we have been bitten before by subtle issues in this kind of thing slipping in and causing problems far down the road. But this is also a blocker for parity with GCC on Linux kernel builds in particular and the patch author has gone through a lot of rounds of feedback already, so I'd be quite keen to land this and follow-up with refactoring patches, if my understanding of the situation is correct.

[jrtc27]

I mean, I did comment on the interface 2.5 months ago, that’s not new feedback

[craig.topper]

Looks like reducing the Streamer interface from 3 down to 1 would require refactoring the code to collect the information before sending it to the streamer. I suggest this be moved to a future patch for expediency of getting this landed since the code has already been reviewed. As long as we're the only target using it, it doesn't seem hard to change. I'm willing to help @StephenFan improve this post-commit.

[hiraditya]

In D123515#4372698, @craig.topper wrote:

Looks like reducing the Streamer interface from 3 down to 1 would require refactoring the code to collect the information before sending it to the streamer. I suggest this be moved to a future patch for expediency of getting this landed since the code has already been reviewed. As long as we're the only target using it, it doesn't seem hard to change. I'm willing to help @StephenFan improve this post-commit.

SGTM, Thanks for offering to help Craig!

[StephenFan]

Thank you all! Considering Android developers are really anxious to get this, I will land it tonight if there are no strong objections.

[jrtc27]

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

94

This wasn't fixed prior to (or while) committing; bacb14b9f31ede0572af504407696c549362c874

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

#include "RISCVGenAsmMatcher.inc"
 
   bool parseOperand(OperandVector &Operands, StringRef Mnemonic);
 
   bool parseDirectiveOption();
   bool parseDirectiveAttribute();
   bool parseDirectiveInsn(SMLoc L);
   bool parseDirectiveVariantCC();
 
+  /// Helper to reset target features for a new arch string. It
+  /// also records the new arch string that is expanded by RISCVISAInfo
+  /// and reports error for invalid arch string.
+  bool resetToArch(StringRef Arch, SMLoc Loc, std::string &Result,
+                   bool FromOptionDirective);
+
   void setFeatureBits(uint64_t Feature, StringRef FeatureString) {
     if (!(getSTI().hasFeature(Feature))) {
       MCSubtargetInfo &STI = copySTI();
       setAvailableFeatures(
           ComputeAvailableFeatures(STI.ToggleFeature(FeatureString)));
     }
   }
 
bool RISCVAsmParser::ParseDirective(AsmToken DirectiveID) {
   if (IDVal == ".insn")
     return parseDirectiveInsn(DirectiveID.getLoc());
   if (IDVal == ".variant_cc")
     return parseDirectiveVariantCC();
 
   return true;
 }
 
+bool RISCVAsmParser::resetToArch(StringRef Arch, SMLoc Loc, std::string &Result,
+                                 bool FromOptionDirective) {
+  for (auto Feature : RISCVFeatureKV)
+    if (llvm::RISCVISAInfo::isSupportedExtensionFeature(Feature.Key))
+      clearFeatureBits(Feature.Value, Feature.Key);
+
+  auto ParseResult = llvm::RISCVISAInfo::parseArchString(
+      Arch, /*EnableExperimentalExtension=*/true,
+      /*ExperimentalExtensionVersionCheck=*/true);
+  if (!ParseResult) {
+    std::string Buffer;
+    raw_string_ostream OutputErrMsg(Buffer);
+    handleAllErrors(ParseResult.takeError(), [&](llvm::StringError &ErrMsg) {
+      OutputErrMsg << "invalid arch name '" << Arch << "', "
+                   << ErrMsg.getMessage();
+    });
+
+    return Error(Loc, OutputErrMsg.str());
+  }
+  auto &ISAInfo = *ParseResult;
+
+  for (auto Feature : RISCVFeatureKV)
+    if (ISAInfo->hasExtension(Feature.Key))
+      setFeatureBits(Feature.Value, Feature.Key);
+
+  if (FromOptionDirective) {
+    if (ISAInfo->getXLen() == 32 && isRV64())
+      return Error(Loc, "bad arch string switching from rv64 to rv32");
+    else if (ISAInfo->getXLen() == 64 && !isRV64())
+      return Error(Loc, "bad arch string switching from rv32 to rv64");
+  }
+
+  if (ISAInfo->getXLen() == 32)
+    clearFeatureBits(RISCV::Feature64Bit, "64bit");
+  else if (ISAInfo->getXLen() == 64)
+    setFeatureBits(RISCV::Feature64Bit, "64bit");
+  else
+    return Error(Loc, "bad arch string " + Arch);
+
+  Result = ISAInfo->toString();
+  return false;
+}
+
 bool RISCVAsmParser::parseDirectiveOption() {
   MCAsmParser &Parser = getParser();
   // Get the option token.
   AsmToken Tok = Parser.getTok();
 
   // At the moment only identifiers are supported.
   if (parseToken(AsmToken::Identifier, "expected identifier"))
     return true;
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)) {
+        IsAdd = true;
+        IsExtensionList = true;
+      } else if (Parser.getTok().is(AsmToken::Minus)) {
+        IsAdd = false;
+        IsExtensionList = true;
+      } else {
+        SMLoc ArchLoc = Parser.getTok().getLoc();
+
+        if (IsExtensionList)
+          return Error(ArchLoc, "unexpected token, expected + or -");
+
+        StringRef Arch;
+        if (Parser.getTok().is(AsmToken::Identifier))
+          Arch = Parser.getTok().getString();
+        else
+          return Error(ArchLoc,
+                       "unexpected token, expected identifier");
+
+        std::string Result;
+        if (resetToArch(Arch, ArchLoc, Result, true))
+          return true;
+
+        getTargetStreamer().emitDirectiveOptionArchFullArch(Result,
+                                                            PrefixEmitted);
+
+        Parser.Lex();
+
+        return Parser.parseToken(AsmToken::EndOfStatement,
+                                 "unexpected token, expected end of statement");
+      }
+
+      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()))
+          return Error(
+              Parser.getTok().getLoc(),
+              "Extension version number parsing not currently implemented");
+        return Error(Parser.getTok().getLoc(), "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) {
+            OutputErrMsg << ErrMsg.getMessage();
+          });
+
+          return Error(Loc, OutputErrMsg.str());
+        }
+        getTargetStreamer().emitDirectiveOptionArchPlus(Ext->Key, PrefixEmitted,
+                                                        HasComma);
+      } else {
+        // 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().emitDirectiveOptionArchMinus(
+            Ext->Key, PrefixEmitted, HasComma);
+      }
+
+      if (!HasComma)
+        return Parser.parseToken(AsmToken::EndOfStatement,
+                                 "unexpected token, expected end of statement");
+      // Eat comma
+      Parser.Lex();
+    }
+  }
+
   if (Option == "rvc") {
     if (Parser.parseEOL())
       return true;
 
     getTargetStreamer().emitDirectiveOptionRVC();
     setFeatureBits(RISCV::FeatureStdExtC, "c");
     return false;
   }
if (Parser.parseEOL())
       return true;
 
     getTargetStreamer().emitDirectiveOptionNoRelax();
     clearFeatureBits(RISCV::FeatureRelax, "relax");
     return false;
   }
 
   // Unknown option.
-  Warning(Parser.getTok().getLoc(),
-          "unknown option, expected 'push', 'pop', 'rvc', 'norvc', 'relax' or "
+  Warning(Parser.getTok().getLoc(), "unknown option, expected 'push', 'pop', "
+                                    "'rvc', 'norvc', 'arch', 'relax' or "
                                     "'norelax'");
   Parser.eatToEndOfStatement();
   return false;
 }
 
 /// parseDirectiveAttribute
 ///  ::= .attribute expression ',' ( expression | "string" )
 ///  ::= .attribute identifier ',' ( expression | "string" )
 bool RISCVAsmParser::parseDirectiveAttribute() {
bool RISCVAsmParser::parseDirectiveAttribute() {
   if (Parser.parseEOL())
     return true;
 
   if (IsIntegerValue)
     getTargetStreamer().emitAttribute(Tag, IntegerValue);
   else if (Tag != RISCVAttrs::ARCH)
     getTargetStreamer().emitTextAttribute(Tag, StringValue);
   else {
-    StringRef Arch = StringValue;
-    for (auto Feature : RISCVFeatureKV)
-      if (llvm::RISCVISAInfo::isSupportedExtensionFeature(Feature.Key))
-        clearFeatureBits(Feature.Value, Feature.Key);
-
-    auto ParseResult = llvm::RISCVISAInfo::parseArchString(
-        StringValue, /*EnableExperimentalExtension=*/true,
-        /*ExperimentalExtensionVersionCheck=*/true);
-    if (!ParseResult) {
-      std::string Buffer;
-      raw_string_ostream OutputErrMsg(Buffer);
-      handleAllErrors(ParseResult.takeError(), [&](llvm::StringError &ErrMsg) {
-        OutputErrMsg << "invalid arch name '" << Arch << "', "
-                     << ErrMsg.getMessage();
-      });
-
-      return Error(ValueExprLoc, OutputErrMsg.str());
-    }
-    auto &ISAInfo = *ParseResult;
-
-    for (auto Feature : RISCVFeatureKV)
-      if (ISAInfo->hasExtension(Feature.Key))
-        setFeatureBits(Feature.Value, Feature.Key);
-
-    if (ISAInfo->getXLen() == 32)
-      clearFeatureBits(RISCV::Feature64Bit, "64bit");
-    else if (ISAInfo->getXLen() == 64)
-      setFeatureBits(RISCV::Feature64Bit, "64bit");
-    else
-      return Error(ValueExprLoc, "bad arch string " + Arch);
+    std::string Result;
+    if (resetToArch(StringValue, ValueExprLoc, Result, false))
+      return true;
 
     // Then emit the arch string.
-    getTargetStreamer().emitTextAttribute(Tag, ISAInfo->toString());
+    getTargetStreamer().emitTextAttribute(Tag, Result);
   }
 
   return false;
 }
 
 bool isValidInsnFormat(StringRef Format, bool AllowC) {
   return StringSwitch<bool>(Format)
       .Cases("r", "r4", "i", "b", "sb", "u", "j", "uj", "s", true)

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

public:
   virtual void emitDirectiveOptionPop();
   virtual void emitDirectiveOptionPIC();
   virtual void emitDirectiveOptionNoPIC();
   virtual void emitDirectiveOptionRVC();
   virtual void emitDirectiveOptionNoRVC();
   virtual void emitDirectiveOptionRelax();
   virtual void emitDirectiveOptionNoRelax();
   virtual void emitDirectiveVariantCC(MCSymbol &Symbol);
+  virtual void emitDirectiveOptionArchFullArch(StringRef Value,
+                                               bool &PrefixEmitted);
+  virtual void emitDirectiveOptionArchPlus(StringRef Value, bool &PrefixEmitted,
+                                           bool EmitComma);
+  virtual void emitDirectiveOptionArchMinus(StringRef Value,
+                                            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 emitDirectiveOptionPop() override;
   void emitDirectiveOptionPIC() override;
   void emitDirectiveOptionNoPIC() override;
   void emitDirectiveOptionRVC() override;
   void emitDirectiveOptionNoRVC() override;
   void emitDirectiveOptionRelax() override;
   void emitDirectiveOptionNoRelax() override;
   void emitDirectiveVariantCC(MCSymbol &Symbol) override;
+  void emitDirectiveOptionArchFullArch(StringRef Value,
+                                       bool &PrefixEmitted) override;
+  void emitDirectiveOptionArchPlus(StringRef Value, bool &PrefixEmitted,
+                                   bool EmitComma) override;
+  void emitDirectiveOptionArchMinus(StringRef Value, bool &PrefixEmitted,
+                                    bool EmitComma) override;
 };
 
 }
 #endif

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

 void RISCVTargetStreamer::emitDirectiveOptionPop() {}
 void RISCVTargetStreamer::emitDirectiveOptionPIC() {}
 void RISCVTargetStreamer::emitDirectiveOptionNoPIC() {}
 void RISCVTargetStreamer::emitDirectiveOptionRVC() {}
 void RISCVTargetStreamer::emitDirectiveOptionNoRVC() {}
 void RISCVTargetStreamer::emitDirectiveOptionRelax() {}
 void RISCVTargetStreamer::emitDirectiveOptionNoRelax() {}
 void RISCVTargetStreamer::emitDirectiveVariantCC(MCSymbol &Symbol) {}
+void RISCVTargetStreamer::emitDirectiveOptionArchFullArch(StringRef Value,
+                                                          bool &hasDotOption) {}
+void RISCVTargetStreamer::emitDirectiveOptionArchPlus(StringRef Value,
+                                                      bool &hasDotOption,
+                                                      bool EmitComma) {}
+void RISCVTargetStreamer::emitDirectiveOptionArchMinus(StringRef Value,
+                                                       bool &hasDotOption,
+                                                       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::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::emitDirectiveOptionArchPlus(StringRef Value,
+                                                         bool &PrefixEmitted,
+                                                         bool EmitComma) {
+  emitDirectiveOptionArchPrefix(OS, PrefixEmitted);
+  OS << "+" << Value;
+  emitCommaOrNextLine(OS, EmitComma);
+}
+void RISCVTargetAsmStreamer::emitDirectiveOptionArchMinus(StringRef Value,
+                                                          bool &PrefixEmitted,
+                                                          bool EmitComma) {
+  emitDirectiveOptionArchPrefix(OS, PrefixEmitted);
+  OS << "-" << Value;
+  emitCommaOrNextLine(OS, EmitComma);
+}
+
 void RISCVTargetAsmStreamer::finishAttributeSection() {}

llvm/test/CodeGen/RISCV/inline-option-directive.ll

+; RUN: llc -mtriple=riscv64 < %s -o - | FileCheck --check-prefixes=CHECK-ATTRIBUTES %s
+; RUN: llc -mtriple=riscv64 < %s -filetype=obj | llvm-readelf -h - \
+; RUN:     | FileCheck --check-prefixes=CHECK-EFLAGS %s
+
+; CHECK-ATTRIBUTES: .attribute      5, "rv64i2p1"
+; CHECK-EFLAGS: Flags: 0x0
+define void @test() {
+  tail call void asm ".option arch, +c", ""()
+  ret void
+}

llvm/test/MC/RISCV/eflags-option-directive.s

+# RUN: llvm-mc -triple=riscv64 -filetype=obj %s -o - | llvm-readelf -h - \
+# RUN:         | FileCheck --check-prefixes=CHECK %s
+
+# CHECK: Flags: 0x0
+.option arch, +c

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

+# RUN: llvm-mc -triple riscv32 -show-encoding < %s \
+# RUN:   | FileCheck -check-prefixes=CHECK %s
+# RUN: llvm-mc -triple riscv32 -filetype=obj < %s \
+# RUN:   | llvm-objdump  --triple=riscv32 --mattr=+c,+m,+a,+f,+zba -d -M no-aliases - \
+# RUN:   | FileCheck -check-prefixes=CHECK-INST %s
+
+# Test '.option arch, +' and '.option arch, -' directive
+# The following test cases were copied from MC/RISCV/option-rvc.s
+
+# CHECK-INST: addi a0, a1, 0
+# CHECK: # encoding:  [0x13,0x85,0x05,0x00]
+addi a0, a1, 0
+
+# CHECK-INST: addi s0, sp, 1020
+# CHECK: # encoding:  [0x13,0x04,0xc1,0x3f]
+addi s0, sp, 1020
+
+# CHECK: .option arch, +c
+.option arch, +c
+# CHECK-INST: c.mv a0, a1
+# CHECK: # encoding:  [0x2e,0x85]
+addi a0, a1, 0
+
+# CHECK-INST: c.addi4spn s0, sp, 1020
+# CHECK: # encoding:  [0xe0,0x1f]
+addi s0, sp, 1020
+
+# CHECK: .option arch, -c
+.option arch, -c
+# CHECK-INST: addi a0, a1, 0
+# CHECK: # encoding:  [0x13,0x85,0x05,0x00]
+addi a0, a1, 0
+
+# CHECK-INST: addi s0, sp, 1020
+# CHECK: # encoding:  [0x13,0x04,0xc1,0x3f]
+addi s0, sp, 1020
+
+# CHECK: .option arch, +c
+.option arch, +c
+# CHECK-INST: c.mv a0, a1
+# CHECK: # encoding:  [0x2e,0x85]
+addi a0, a1, 0
+
+# CHECK-INST: c.addi4spn s0, sp, 1020
+# CHECK: # encoding:  [0xe0,0x1f]
+addi s0, sp, 1020
+
+# CHECK: .option arch, -c
+.option arch, -c
+# CHECK-INST: addi a0, a1, 0
+# CHECK: # encoding:  [0x13,0x85,0x05,0x00]
+addi a0, a1, 0
+
+# CHECK-INST: addi s0, sp, 1020
+# CHECK: # encoding:  [0x13,0x04,0xc1,0x3f]
+addi s0, sp, 1020
+
+# CHECK: .option arch, +d, -d
+.option arch, +d, -d
+# CHECK-INST: flw ft0, 12(a0)
+# CHECK: # encoding:  [0x07,0x20,0xc5,0x00]
+flw f0, 12(a0)
+
+# Test comma-separated list
+# CHECK: arch, +m, +a
+.option arch, +m, +a
+# CHECK-INST: mul a4, ra, s0
+# CHECK: # encoding:  [0x33,0x87,0x80,0x02]
+mul a4, ra, s0
+# CHECK-INST: lr.w t0, (t1)
+# CHECK: # encoding:  [0xaf,0x22,0x03,0x10]
+lr.w t0, (t1)
+
+# Test multi-letter extension
+# CHECK: .option arch, +zba
+.option arch, +zba
+# CHECK-INST: sh1add t0, t1, t2
+# CHECK: encoding: [0xb3,0x22,0x73,0x20]
+sh1add t0, t1, t2
+
+# Test '.option arch, <arch-string>' directive
+# CHECK: .option arch, rv32i2p1_m2p0_a2p1_c2p0
+.option arch, rv32i2p1_m2p0_a2p1_c2p0
+
+# CHECK-INST: mul a4, ra, s0
+# CHECK: # encoding:  [0x33,0x87,0x80,0x02]
+mul a4, ra, s0
+# CHECK-INST: lr.w t0, (t1)
+# CHECK: # encoding:  [0xaf,0x22,0x03,0x10]
+lr.w t0, (t1)
+
+# Test arch string without version number
+#CHECK: .option arch, rv32i2p1_m2p0_a2p1_c2p0
+.option arch, rv32imac
+# CHECK-INST: mul a4, ra, s0
+# CHECK: # encoding:  [0x33,0x87,0x80,0x02]
+mul a4, ra, s0
+# CHECK-INST: lr.w t0, (t1)
+# CHECK: # encoding:  [0xaf,0x22,0x03,0x10]
+lr.w t0, (t1)
+
+# Test +c, -c and vice-versa
+.option arch, +c, -c
+# CHECK-INST: addi a0, a1, 0
+# CHECK: # encoding:  [0x13,0x85,0x05,0x00]
+addi a0, a1, 0
+
+.option arch, -c, +c
+# CHECK-INST: c.mv a0, a1
+# CHECK: # encoding:  [0x2e,0x85]
+addi a0, a1, 0
+
+.option arch, rv32ic
+# CHECK-INST: c.mv a0, a1
+# CHECK: # encoding:  [0x2e,0x85]
+addi a0, a1, 0
+
+# Test extension name that has digits.
+.option arch, +zve32x
+# CHECK: .option arch, +zve32x

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

 # RUN: not llvm-mc -triple riscv32 < %s 2>&1 \
 # RUN:   | FileCheck -check-prefixes=CHECK %s
 
 # CHECK: :[[#@LINE+1]]:8: error: expected identifier
 .option
 
 # CHECK: :[[#@LINE+1]]:9: error: expected identifier
 .option 123
 
 # CHECK: :[[#@LINE+1]]:9: error: expected identifier
 .option "str"
 
 # CHECK: :[[#@LINE+1]]:13: error: expected newline
 .option rvc foo
 
-# CHECK: :[[#@LINE+1]]:12: warning: unknown option, expected 'push', 'pop', 'rvc', 'norvc', 'relax' or 'norelax'
+# 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
+.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
+.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
+
+# CHECK: :[[#@LINE+1]]:16: error: Can't disable zicsr extension, f extension requires zicsr extension be enabled
+.option arch, -zicsr
+
+# CHECK: :[[#@LINE+1]]:20: error: 'f' and 'zfinx' extensions are incompatible
+.option arch, +f, +zfinx
+
+# CHECK: :[[#@LINE+1]]:13: error: expected newline
+.option rvc foo
+
+# CHECK: :[[#@LINE+1]]:12: warning: unknown option, expected 'push', 'pop', 'rvc', 'norvc', 'arch', 'relax' or 'norelax'
 .option bar
 
+# CHECK: :[[#@LINE+1]]:16: error: unknown extension feature
+.option arch, -i
+
 # CHECK: :[[#@LINE+1]]:12: error: .option pop with no .option push
 .option pop
 
 # CHECK: :[[#@LINE+1]]:14: error: expected newline
 .option push 123
 
 # CHECK: :[[#@LINE+1]]:13: error: expected newline
 .option pop 123
+
+# CHECK: :[[#@LINE+1]]:15: error: bad arch string switching from rv32 to rv64
+.option arch, rv64gc