[PATCH] D123515: [RISCV] Support '.option arch' directive

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2476
+
+      if (ExtStr.find_if(isDigit) != StringRef::npos)
+        return Error(
----------------
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.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2495
+        // Check dependency. It is invalid to disable an extension that
+        // there are other enabled extensions depend on it.
+        for (auto Feature : KVArray) {
----------------
"It is invalid to disable an extension if there are other enabled extensions that depend on it."


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2498
+          if (getSTI().hasFeature(Feature.Value) &&
+              Feature.Implies.test(Ext->Value))
+            return Error(Parser.getTok().getLoc(),
----------------
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`.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp:150
+}
+void RISCVTargetAsmStreamer::emitDirectiveOptionArchPlus(StringRef Value,
+                                                         bool &PrefixEmitted,
----------------
Can emitDirectiveOptionArchPlus/emitDirectiveOptionArchMinus be combined by passing a bool for + or -?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123515/new/

https://reviews.llvm.org/D123515