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

[Kito Cheng via Phabricator via llvm-commits]

kito-cheng added inline comments.


================
Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:31
+public:
+  RISCVISAInfo() : XLen(0), FLen(0) {}
+
----------------
jrtc27 wrote:
> Does Exts need initialising to be empty here? I can never remember
std::map has default construct that will construct an empty map.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:194
+  switch (ExtClass) {
+  case 's':
+    HighOrder = 0;
----------------
jrtc27 wrote:
> 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
I would prefer submit another patch to make this parser up to date, and keep this patch as a refactor patch as possible:

e.g.:
- Full implication info, e.g. `d` implied `f`, `f` implied `zicsr`...
- Prefix `zxm`.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:197
+    break;
+  case 'h':
+    HighOrder = 1;
----------------
jrtc27 wrote:
> 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-.
Like above reply, prefer another patch to fix that.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:225-229
+  if (LHSLen == 1 && RHSLen != 1)
+    return true;
+
+  if (LHSLen != 1 && RHSLen == 1)
+    return false;
----------------
jrtc27 wrote:
> Don't know if this is better or not, but this is the more compact way to write it
It's more compact but it's hard to read the rule to me, so I would prefer keep that as it is


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168