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

[Kito Cheng via Phabricator via cfe-commits]

kito-cheng marked 13 inline comments as done.
kito-cheng added inline comments.


================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:230
+  unsigned XLEN = getTriple().isArch64Bit() ? 64 : 32;
+  if (auto E = ISAInfo.parse(XLEN, Features))
+    return false;
----------------
jrtc27 wrote:
> Unused variable; I think this should either be consumeError'ed or propagated up?
Change return type to `void`, we only checking XLEN inside parse function for feature vector, so change it to an assertion.


================
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;
----------------
craig.topper wrote:
> Why is A treated differently?
Removed, look-up ISAInfo instead.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:117
+static Optional<RISCVExtensionVersion> isExperimentalExtension(StringRef Ext) {
+  for (auto const &SupportedExtensionInfo : SupportedExtensionInfos) {
+    if (!SupportedExtensionInfo.Experimental)
----------------
craig.topper wrote:
> 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?
I add an helper function `filterSupportedExtensionInfosByName` here rather than `findExtensionByName`, since I suppose we might support multiple version for one extension in future, e.g. `F` with version `2.0`, `2.1`, `2.2`, so I think keep a loop here might be easier to extend.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:625
+  SmallVector<StringRef, 8> AllExts;
+  SmallVector<StringRef, 4> Prefix{"z", "x", "s", "sx"};
+  auto I = Prefix.begin();
----------------
craig.topper wrote:
> 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
Using std::arrary here, the cost should be cheap as built-in/plain array, but with utils functions.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:644
+                               "invalid extension prefix '%s'",
+                               Ext.str().c_str());
+    }
----------------
jrtc27 wrote:
> craig.topper wrote:
> > craig.topper wrote:
> > > does createStringError really require going to from StringRef to std::string to char*?
> > I guess it does if you use it with %s. Maybe better to use 
> > 
> > ```
> > "invalid extension prefix '" + Ext + "'"
> > ```
> > 
> > since the format string is really a Twine.
> That will break if the user-provided extension contains a %. Sadly `.str().c_str()` is what you need.
Seems like `"invalid extension prefix '" + Ext + "'"` is work, createStringError has an overload is accept Twine as argument, and no format string interpretation 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168