This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Extend getTargetDefines for RISCVTargetInfo
ClosedPublic

Authored by kito-cheng on Mar 20 2018, 10:28 PM.

Details

Summary

This patch extend getTargetDefines and implement handleTargetFeatures and hasFeature. and define corresponding marco for those features.

Diff Detail

Repository
rC Clang

Event Timeline

kito-cheng created this revision.Mar 20 2018, 10:28 PM

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.

apazos added inline comments.Mar 27 2018, 10:38 AM
test/Preprocessor/riscv-target-features.c
10

Consider breaking the long lines to be within 80 cols.

kito-cheng retitled this revision from [RISCV] Implement getTargetDefines, handleTargetFeatures and hasFeature for RISCVTargetInfo to [RISCV] Extend getTargetDefines for RISCVTargetInfo.
kito-cheng edited the summary of this revision. (Show Details)

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.
kito-cheng marked 5 inline comments as done.Mar 28 2018, 12:12 AM

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.

kito-cheng updated this revision to Diff 140913.Apr 4 2018, 1:11 AM

Changes:

  • Add testcase for TargetFeature: riscv, riscv32 and riscv64

Hi Eli:

Thanks your advise, I've checked those marco are match with GCC :)

asb added a comment.Apr 4 2018, 7:58 AM

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.

kito-cheng updated this revision to Diff 140959.Apr 4 2018, 8:09 AM

Changes:

  • Reorder marco define into canonical order which specified in ISA manual.
  • Add missing test for marco.
kito-cheng marked an inline comment as done.Apr 4 2018, 8:10 AM
asb accepted this revision.Apr 4 2018, 8:31 AM

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");
}
This revision is now accepted and ready to land.Apr 4 2018, 8:31 AM
This revision was automatically updated to reflect the committed changes.