craig.topper added inline comments.
================
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;
----------------
Why is A treated differently?
================
Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:35
+ // Helper class for OrderedExtensionMap.
+ struct ExtensionComparetor {
+ bool operator()(const std::string &LHS, const std::string &RHS) const {
----------------
Comparetor -> Comparator
================
Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:39
+ }
+ };
+ // OrderedExtensionMap is a StrinMap-like container, but specialized for
----------------
StrinMap->StringMap
================
Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:40
+ };
+ // OrderedExtensionMap is a StrinMap-like container, but specialized for
+ // keep entries in canonical order of extension.
----------------
"for keep" -> "to keep"
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:72
+// return true if did the strip.
+static bool StripExperimentalPrefix(StringRef &Ext) {
+ StringRef ExperimentalPrefix = "experimental-";
----------------
Please rename to stripExperimentalPrefix per the clang-tidy warning
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:74
+ StringRef ExperimentalPrefix = "experimental-";
+ if (Ext.startswith(ExperimentalPrefix)) {
+ Ext = Ext.drop_front(ExperimentalPrefix.size());
----------------
```
return Ext.consume_front("experimental-")
```
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:117
+static Optional<RISCVExtensionVersion> isExperimentalExtension(StringRef Ext) {
+ for (auto const &SupportedExtensionInfo : SupportedExtensionInfos) {
+ if (!SupportedExtensionInfo.Experimental)
----------------
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?
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:185
+ size_t Pos = AllStdExts.find(Ext);
+ int rank;
+ if (Pos == StringRef::npos)
----------------
rank -> Rank
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:432
+ }
+ bool HasRV64 = Arch.startswith("rv64");
+
----------------
Check this before the if, and use HasRV64 in it's place in the if so we don't
duplicate the same search.
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:622
+ SmallVector<StringRef, 8> Split;
+ OtherExts.split(Split, StringRef("_"));
+
----------------
Isn't there a split that takes a char for the Separator instead of StringRef.
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:625
+ SmallVector<StringRef, 8> AllExts;
+ SmallVector<StringRef, 4> Prefix{"z", "x", "s", "sx"};
+ auto I = Prefix.begin();
----------------
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
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:644
+ "invalid extension prefix '%s'",
+ Ext.str().c_str());
+ }
----------------
does createStringError really require going to from StringRef to std::string to
char*?
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
