[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:33
+  bool operator!=(const RISCVExtensionVersion &Version) const {
+    return !operator==(Version);
+  }
----------------
Use `!(*this == Version)`


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:154
 
-static Optional<RISCVExtensionVersion> isExperimentalExtension(StringRef Ext) {
-  auto ExtIterator =
-      llvm::find_if(SupportedExperimentalExtensions, FindByName(Ext));
-  if (ExtIterator == std::end(SupportedExperimentalExtensions))
-    return None;
+static std::vector<RISCVExtensionVersion> isExperimentalExtension(StringRef Ext) {
+  std::vector<RISCVExtensionVersion> Result;
----------------
This name was already bad and getting worse. It doesn't return a bool so shouldn't start with `is`. It should be called something like `getExperimentalExtensionVersions`. 


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:341
+          addVersionSuffix("experimental-", "zvlsseg", Major, Minor));
+    } else if (isExperimentalExtension(ExtName).size()) {
+      Features.push_back(
----------------
Use `!isExperimentalExtension(ExtName).empty()`


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:404
+  auto ExperimentalExtension = isExperimentalExtension(Ext);
+  if (ExperimentalExtension.size()) {
     if (!EnableExperimentalExtension) {
----------------
!empty


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:481
+    auto Info = isSupportedExtensionFeature(ExtName);
+    if(!Info)
       continue;
----------------
Please fix this clang-format issue


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.cpp:56
+void RISCVSubtarget::initializeEnvironment() {
+  StdExtM = {0, 0};
+  StdExtA = {0, 0};
----------------
Add a reset  method?


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.cpp:91
                                                 StringRef ABIName) {
+  initializeEnvironment();
   // Determine default and user-specified characteristics
----------------
Why do we need to initialize things now but didn't before?


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:38
   virtual void anchor();
-  bool HasStdExtM = false;
-  bool HasStdExtA = false;
-  bool HasStdExtF = false;
-  bool HasStdExtD = false;
-  bool HasStdExtC = false;
-  bool HasStdExtZba = false;
-  bool HasStdExtZbb = false;
-  bool HasStdExtZbc = false;
-  bool HasStdExtZbe = false;
-  bool HasStdExtZbf = false;
-  bool HasStdExtZbm = false;
-  bool HasStdExtZbp = false;
-  bool HasStdExtZbr = false;
-  bool HasStdExtZbs = false;
-  bool HasStdExtZbt = false;
-  bool HasStdExtV = false;
-  bool HasStdExtZvlsseg = false;
-  bool HasStdExtZfhmin = false;
-  bool HasStdExtZfh = false;
+  RISCVExtensionVersion StdExtM = {0, 0};
+  RISCVExtensionVersion StdExtA = {0, 0};
----------------
Can we have a default constructor for RISCVExtensionVersion?


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:106
   bool enableMachineScheduler() const override { return true; }
-  bool hasStdExtM() const { return HasStdExtM; }
-  bool hasStdExtA() const { return HasStdExtA; }
-  bool hasStdExtF() const { return HasStdExtF; }
-  bool hasStdExtD() const { return HasStdExtD; }
-  bool hasStdExtC() const { return HasStdExtC; }
-  bool hasStdExtZba() const { return HasStdExtZba; }
-  bool hasStdExtZbb() const { return HasStdExtZbb; }
-  bool hasStdExtZbc() const { return HasStdExtZbc; }
-  bool hasStdExtZbe() const { return HasStdExtZbe; }
-  bool hasStdExtZbf() const { return HasStdExtZbf; }
-  bool hasStdExtZbm() const { return HasStdExtZbm; }
-  bool hasStdExtZbp() const { return HasStdExtZbp; }
-  bool hasStdExtZbr() const { return HasStdExtZbr; }
-  bool hasStdExtZbs() const { return HasStdExtZbs; }
-  bool hasStdExtZbt() const { return HasStdExtZbt; }
-  bool hasStdExtV() const { return HasStdExtV; }
-  bool hasStdExtZvlsseg() const { return HasStdExtZvlsseg; }
-  bool hasStdExtZfhmin() const { return HasStdExtZfhmin; }
-  bool hasStdExtZfh() const { return HasStdExtZfh; }
+  bool hasStdExtM() const { return StdExtM != RISCVExtensionVersion{0, 0}; }
+  bool hasStdExtA() const { return StdExtA != RISCVExtensionVersion{0, 0}; }
----------------
Add a helper method to RISCVExtensionVersion to check for a null version?


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

https://reviews.llvm.org/D115921