[PATCH] D44727: [RISCV] Implement getTargetDefines, handleTargetFeatures and hasFeature for RISCVTargetInfo

[Alex Bradbury via Phabricator via cfe-commits]

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