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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.


================
Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:31
+public:
+  RISCVISAInfo() : XLen(0), FLen(0) {}
+
----------------
Does Exts need initialising to be empty here? I can never remember


================
Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:47-52
+  /// Parse RISCV ISA info from arch string.
+  Error parseArchString(StringRef Arch, bool EnableExperimentalExtension,
+                        bool ExperimentalExtensionVersionCheck = true);
+
+  /// Parse RISCV ISA info from feature vector.
+  void parseFeatures(unsigned XLen, const std::vector<std::string> &Features);
----------------
I wonder, should these be constructors instead? `RISCVISAInfo ISAInfo; ISAInfo.parse` doesn't feel very idiomatic C++


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:178
+    // order, but after all known standard extensions.
+    Rank = AllStdExts.size() + (Ext - 'a');
+  else
----------------
Why not just return these directly? Don't need a variable.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:194
+  switch (ExtClass) {
+  case 's':
+    HighOrder = 0;
----------------
I imagine this needs to change to the new Sm-/Ss- scheme, but I don't know which way round those are intended to go


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:197
+    break;
+  case 'h':
+    HighOrder = 1;
----------------
Do we know if this is still the case? Ss- is being used for S-mode extensions and Sm- for M-mode extensions, so I'd expect Sh- (or maybe just Ss-) for HS-mode extensions, not H-.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:207
+  default:
+    assert(false && "Unknown prefix for multi-char extension");
+    return -1;
----------------
llvm_unreachable


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:214
+  if (ExtClass == 'z')
+    LowOrder = singleLetterExtensionRank(ExtName[1]);
+
----------------
Why not put this in its switch case?


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:225-229
+  if (LHSLen == 1 && RHSLen != 1)
+    return true;
+
+  if (LHSLen != 1 && RHSLen == 1)
+    return false;
----------------
Don't know if this is better or not, but this is the more compact way to write it


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:231
+  size_t RHSLen = RHS.length();
+  int LHSRank, RHSRank;
+  if (LHSLen == 1 && RHSLen != 1)
----------------
frasercrmck wrote:
> Not sure why these need to be declared up here.
Yeah, declare the variables at their assignments. Besides, you don't even need variables for the single-letter case, just compare the return values of the functions directly.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:40
+
+static const StringRef AllStdExts = "mafdqlcbjtpvn";
+
----------------
kito-cheng wrote:
> jrtc27 wrote:
> > craig.topper wrote:
> > > Make this `static constexpr StringLiteral AllStdExts = "mafdqlcbjtpvn";`
> > I can't help but feel this is really an array of chars, not a string. We don't even need the trailing NUL, though double quote syntax is simpler than curly braces and a bunch of single-quote chars just to save a byte.
> Yeah, it's actually just an array of chars, but we have use find function from StringRef :p
Still not a StringLiteral


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:43
+static const RISCVSupportedExtensionInfo SupportedExtensionInfos[] = {
+    {"i", RISCVExtensionVersion{2, 0}, /* Experimental */ false},
+    {"e", RISCVExtensionVersion{1, 9}, /* Experimental */ false},
----------------
jrtc27 wrote:
> but that's the default so I'd omit it for anything other than the cases where it's true
I do think it would be better to not list Experimental for the non-experimental ones


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168