This patch extend getTargetDefines and implement handleTargetFeatures and hasFeature. and define corresponding marco for those features.
Details
Details
Diff Detail
Diff Detail
- Repository
- rC Clang
Event Timeline
Comment Actions
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
| lib/Basic/Targets/RISCV.cpp | ||
|---|---|---|
| 62–65 | 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! | |
| 68 | 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. | |
| 78 | 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. | |
| 83 | You could get away without the brackets for these if/else if. | |
| test/Preprocessor/riscv-target-features.c | ||
|---|---|---|
| 10 | Consider breaking the long lines to be within 80 cols. | |
Comment Actions
Changes:
- Define riscv_atomic, riscv_flen, riscv_fdiv and riscv_fsqr, and add test for those marco.
- Handle riscv, riscv32 and riscv64 in RISCVTargetInfo::hasFeature.
- Fix several coding style issue.
- Breaking the long lines in test case.
- Add comment for RISCVTargetInfo::hasFeature.
Comment Actions
Do the macros you're defining here match gcc?
| lib/Basic/Targets/RISCV.cpp | ||
|---|---|---|
| 68 | I think your assessment is right... and also, it looks like we have poor test coverage for the set of features which are actually supported. So probably some targets are missing relevant features. Should be straightforward to test, I think; you just need to write a module.map with appropriate "requires" lines, and a test file which tries to include those modules. See test/Modules/Inputs/module.map. | |
Comment Actions
Thanks Eli for the info on hasFeature.
I think the only thing now missing is test coverage for the defines.
| lib/Basic/Targets/RISCV.cpp | ||
|---|---|---|
| 53–73 | I'd re-order these so features are tested in the canonical RISC-V order: M, then A, then F, then D, then C. | |
Comment Actions
Changes:
- Reorder marco define into canonical order which specified in ISA manual.
- Add missing test for marco.
Comment Actions
Thanks, this looks good to me.
| lib/Basic/Targets/RISCV.cpp | ||
|---|---|---|
| 62–70 | We really are nitpicking now, but this could be written as below, which would keep the F/D defines in one place. if (HasF || HasD) {
Builder.defineMacro("__riscv_flen", HasD ? "64" : "32");
Builder.defineMacro("__riscv_fdiv");
Builder.defineMacro("__riscv_fsqrt");
} | |
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!