kito-cheng marked 13 inline comments as done.
kito-cheng added inline comments.
================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:230
+ unsigned XLEN = getTriple().isArch64Bit() ? 64 : 32;
+ if (auto E = ISAInfo.parse(XLEN, Features))
+ return false;
----------------
jrtc27 wrote:
> Unused variable; I think this should either be consumeError'ed or propagated
> up?
Change return type to `void`, we only checking XLEN inside parse function for
feature vector, so change it to an assertion.
================
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;
----------------
craig.topper wrote:
> Why is A treated differently?
Removed, look-up ISAInfo instead.
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:117
+static Optional<RISCVExtensionVersion> isExperimentalExtension(StringRef Ext) {
+ for (auto const &SupportedExtensionInfo : SupportedExtensionInfos) {
+ if (!SupportedExtensionInfo.Experimental)
----------------
craig.topper wrote:
> 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?
I add an helper function `filterSupportedExtensionInfosByName` here rather than
`findExtensionByName`, since I suppose we might support multiple version for
one extension in future, e.g. `F` with version `2.0`, `2.1`, `2.2`, so I think
keep a loop here might be easier to extend.
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:625
+ SmallVector<StringRef, 8> AllExts;
+ SmallVector<StringRef, 4> Prefix{"z", "x", "s", "sx"};
+ auto I = Prefix.begin();
----------------
craig.topper wrote:
> 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
Using std::arrary here, the cost should be cheap as built-in/plain array, but
with utils functions.
================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:644
+ "invalid extension prefix '%s'",
+ Ext.str().c_str());
+ }
----------------
jrtc27 wrote:
> craig.topper wrote:
> > craig.topper wrote:
> > > does createStringError really require going to from StringRef to
> > > std::string to char*?
> > I guess it does if you use it with %s. Maybe better to use
> >
> > ```
> > "invalid extension prefix '" + Ext + "'"
> > ```
> >
> > since the format string is really a Twine.
> That will break if the user-provided extension contains a %. Sadly
> `.str().c_str()` is what you need.
Seems like `"invalid extension prefix '" + Ext + "'"` is work,
createStringError has an overload is accept Twine as argument, and no format
string interpretation
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
