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