This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Implement new architecture extension macros
ClosedPublic

Authored by simoncook on Jan 11 2021, 4:50 AM.

Details

Summary

This adds support for the new architecture extension test macros as
defined in the C-API Document:
https://github.com/riscv/riscv-c-api-doc/blob/master/riscv-c-api.md

Extension versions have been taken from what are used in
RISCVTargetStreamer for ratified extensions, and the -march parser
for experimental extensions.

Diff Detail

Event Timeline

simoncook created this revision.Jan 11 2021, 4:50 AM
simoncook requested review of this revision.Jan 11 2021, 4:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2021, 4:50 AM
simoncook updated this revision to Diff 315805.Jan 11 2021, 7:33 AM

Correct constant used in macros, 2.0 should be 2000000 not 20000000

khchen accepted this revision.Jan 17 2021, 6:45 PM
khchen added a subscriber: khchen.

LGTM

This revision is now accepted and ready to land.Jan 17 2021, 6:45 PM
kito-cheng added a comment.EditedJan 17 2021, 7:50 PM

Thanks you implement that on clang, I think it's really great to included that in LLVM 12 release.

I would like to define marco for sub-extension too, I know that's my fault, I didn't specify the behavior of sub-extension clearly on the spec, but I think it would be great if we also define sub-extension marcos, since it would be easier to check when some core only implement sub-extension, and the code can just check the sub-extensio rather than check both.

clang/lib/Basic/Targets/RISCV.cpp
148

Could you add all subset included in B, like zba, zbb, zbc, zbp...

153

Could you add all subset included in V, Zvamo and Zvlsseg.

Thanks you implement that on clang, I think it's really great to included that in LLVM 12 release.

I would like to define marco for sub-extension too, I know that's my fault, I didn't specify the behavior of sub-extension clearly on the spec, but I think it would be great if we also define sub-extension marcos, since it would be easier to check when some core only implement sub-extension, and the code can just check the sub-extensio rather than check both.

That's no problem, I'll extend to cover sub-extensions before landing. I notice the zvamo/zvlsseg extensions aren't yet accepted by the -march flag, so I will put a patch together for those and likely land those together.

Rebase on D94930 to show updated version

Just note how current GCC implemented, GCC implement that like implied extension, e.g. V implied Zvamo and Zvlsseg, so __riscv_zvamo is naturally defined when V-ext is enabled.

clang/lib/Basic/Targets/RISCV.cpp
148

I expect there HasB also define __riscv_zbb, __riscv_zbc... as well here.

153

Ditto, I expect there HasV also define __riscv_zvamo, __riscv_zvlsseg... as well here.

simoncook updated this revision to Diff 317591.Jan 19 2021, 8:49 AM

Have 'b'/'v' features imply subfeatures

asb added a comment.Jan 21 2021, 5:18 AM

@kito-cheng could you please confirm that this patch handles sub-extensions in the same way GCC does. i.e. -march=rv32izbb0p92 defines __riscv_zbb but NOT __riscv_b? That seems logical to me, as otherwise it would be cumbersome to check if the whole extension is supported rather than just a subset, but I just wanted to confirm.

kito-cheng accepted this revision.Jan 21 2021, 7:59 AM

I believe the behavior has aligned to GCC now.

In D94403#2512232, @asb wrote:

@kito-cheng could you please confirm that this patch handles sub-extensions in the same way GCC does. i.e. -march=rv32izbb0p92 defines __riscv_zbb but NOT __riscv_b?

Yes :)

simoncook updated this revision to Diff 318869.Jan 24 2021, 2:28 PM
  • Update to bitmanip 0.93
  • Expand and support vector as per workaround in D95146
  • Add negative testing (check __riscv_b not defined for just subextension)
This revision was automatically updated to reflect the committed changes.