[PATCH] D105168: [RISCV] Unify the arch string parsing logic to to RISCVArchStringParser.

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: clang/lib/Basic/Targets/RISCV.h:29
   std::string ABI, CPU;
-  bool HasM = false;
-  bool HasA = false;
-  bool HasF = false;
-  bool HasD = false;
-  bool HasC = false;
-  bool HasB = false;
-  bool HasV = false;
-  bool HasZba = false;
-  bool HasZbb = false;
-  bool HasZbc = false;
-  bool HasZbe = false;
-  bool HasZbf = false;
-  bool HasZbm = false;
-  bool HasZbp = false;
-  bool HasZbproposedc = false;
-  bool HasZbr = false;
-  bool HasZbs = false;
-  bool HasZbt = false;
-  bool HasZfh = false;
-  bool HasZvamo = false;
-  bool HasZvlsseg = false;
-
+  bool HasA;
+  llvm::RISCVISAInfo ISAInfo;
----------------
Why is A treated differently?


================
Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:35
+  // Helper class for OrderedExtensionMap.
+  struct ExtensionComparetor {
+    bool operator()(const std::string &LHS, const std::string &RHS) const {
----------------
Comparetor -> Comparator


================
Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:39
+    }
+  };
+  // OrderedExtensionMap is a StrinMap-like container, but specialized for
----------------
StrinMap->StringMap


================
Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:40
+  };
+  // OrderedExtensionMap is a StrinMap-like container, but specialized for
+  // keep entries in canonical order of extension.
----------------
"for keep" -> "to keep"


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:72
+// return true if did the strip.
+static bool StripExperimentalPrefix(StringRef &Ext) {
+  StringRef ExperimentalPrefix = "experimental-";
----------------
Please rename to stripExperimentalPrefix per the clang-tidy warning


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:74
+  StringRef ExperimentalPrefix = "experimental-";
+  if (Ext.startswith(ExperimentalPrefix)) {
+    Ext = Ext.drop_front(ExperimentalPrefix.size());
----------------
```
return Ext.consume_front("experimental-")
```


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:117
+static Optional<RISCVExtensionVersion> isExperimentalExtension(StringRef Ext) {
+  for (auto const &SupportedExtensionInfo : SupportedExtensionInfos) {
+    if (!SupportedExtensionInfo.Experimental)
----------------
Would it be better to have a findExtensionByName function that contains the loop that can be shared by all these functions. Then you just check the fields of the returned value for the additional checks?


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:185
+  size_t Pos = AllStdExts.find(Ext);
+  int rank;
+  if (Pos == StringRef::npos)
----------------
rank -> Rank


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:432
+  }
+  bool HasRV64 = Arch.startswith("rv64");
+
----------------
Check this before the if, and use HasRV64 in it's place in the if so we don't duplicate the same search.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:622
+  SmallVector<StringRef, 8> Split;
+  OtherExts.split(Split, StringRef("_"));
+
----------------
Isn't there a split that takes a char for the Separator instead of StringRef.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:625
+  SmallVector<StringRef, 8> AllExts;
+  SmallVector<StringRef, 4> Prefix{"z", "x", "s", "sx"};
+  auto I = Prefix.begin();
----------------
Can this be a regular array instead of a SmallVector? You'll need to use std::begin(Prefix) and std::end(Prefix) in place of begin()/end() below


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:644
+                               "invalid extension prefix '%s'",
+                               Ext.str().c_str());
+    }
----------------
does createStringError really require going to from StringRef to std::string to char*?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168