[PATCH] D141479: [RISCV] Generate march string from target features

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added inline comments.


================
Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:30
+
+static std::string getMArch(int XLen, const Record &Rec) {
+  std::vector<Record *> Features = Rec.getValueAsListOfDefs("Features");
----------------
I think you should reference here the pieces of the standard that describe the rules for building the March string.


================
Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:31
+static std::string getMArch(int XLen, const Record &Rec) {
+  std::vector<Record *> Features = Rec.getValueAsListOfDefs("Features");
+  std::vector<std::string> FeatureVector;
----------------
Do you need this variable? it is used only once...


================
Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:38
+    if (llvm::RISCVISAInfo::isSupportedExtensionFeature(FeatureName))
+      FeatureVector.push_back(std::string("+") + FeatureName.str());
+  }
----------------
I think you can avoid constructing a `std::string` here?


================
Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:41
+
+  auto ISAInfo = llvm::RISCVISAInfo::parseFeatures(XLen, FeatureVector);
+  if (!ISAInfo)
----------------
Please avoid `auto` if the type cannot be deducted from the context.


================
Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:66
+      int XLen = getXLen(Rec);
+      std::string EnumFeatures = getEnumFeatures(XLen);
+      std::string MArch = Rec.getValueAsString("DefaultMarch").str();
----------------
Do we need this variable? It is used only once...


================
Comment at: llvm/utils/TableGen/RISCVTargetDefEmitter.cpp:67
+      std::string EnumFeatures = getEnumFeatures(XLen);
+      std::string MArch = Rec.getValueAsString("DefaultMarch").str();
+
----------------
You could use StringRef and then check MArch.empty() in line 70.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141479