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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
