asb added subscribers: doug.gregor, rsmith.
asb added a comment.
Thanks Kito. I've added some comments inline.
Nitpicking: the patch description would be more accurate to say it "extends
getTargetDefines", as obviously an initial implementation was already present
================
Comment at: lib/Basic/Targets/RISCV.cpp:62-65
+ // TODO: Define __riscv_flen, __riscv_fdiv and __riscv_fsqrt after
+ // F and D code gen upstream.
+
+ // TODO: Define __riscv_atomic after A code gen upstream.
----------------
I'd go ahead and define these. At this point, Clang has already accepted a/f/d
in the march string, and even in the case where codegen isn't yet implemented,
MC layer support will be enabled allowing usage in inline assembly. Plus by
doing it now it means we won't forget to come back to this and add those
defines later!
================
Comment at: lib/Basic/Targets/RISCV.cpp:68
+
+bool RISCVTargetInfo::hasFeature(StringRef Feature) const {
+ return llvm::StringSwitch<bool>(Feature)
----------------
It seems a number of other targets also return true for the archname, e.g.
"riscv".
Is it the case that hasFeature should always support at least everything
detected by handleTargetFeatures? If so, a comment to document this would be
helpful.
I can see this method was introduced in rC149227 and is primarily motivated by
module requirements.
@doug.gregor / @rsmith : could you please advise on the implementation and
testing of this function? Which features should be hasFeature check for? I note
that e.g. lib/Basic/Targets/ARM.cpp only supports a subset of the features in
hasFeature compared to handleTargetFeatures.
================
Comment at: lib/Basic/Targets/RISCV.cpp:78
+
+/// handleTargetFeatures - Perform initialization based on the user
+/// configured set of features.
----------------
Nit: current guidance is not to duplicate the method name in the comment
https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
(i.e. you should just comment `Perform initialization based on the user
configured set of features.`
================
Comment at: lib/Basic/Targets/RISCV.cpp:83
+ for (const auto &Feature : Features) {
+ if (Feature == "+m") {
+ HasM = true;
----------------
You could get away without the brackets for these if/else if.
Repository:
rC Clang
https://reviews.llvm.org/D44727
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
