This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Enable __ARM_FEATURE_SVE macros.
ClosedPublic

Authored by sdesmalen on Jun 12 2020, 2:37 AM.

Details

Summary

This patch enables the following macros when their corresponding
target attributes are set:

__ARM_FEATURE_SVE (+sve)
__ARM_FEATURE_SVE2 (+sve2)
__ARM_FEATURE_SVE2_AES (+sve2-aes)
__ARM_FEATURE_SVE2_BITPERM (+sve2-bitperm)
__ARM_FEATURE_SVE2_SHA3 (+sve2-sha3)
__ARM_FEATURE_SVE2_SM4 (+sve2-sm4)

This implies that the base SVE and SVE2 ACLE (00bet2) are now feature
complete, meaning that all intrinsics are implemented in LLVM and Clang.

Disclaimer:

To implement the ACLE we have had to fix up many parts of LLVM to make it
support scalable vectors. We have also used many target-specific intrinsics
to reduce reliance on parts of LLVM where we know scalable vectors may
not yet be handled properly (e.g. some transformation might drop the
'scalable' flag on a vector type). While we've done a best effort with
the limited testing that is available to us, we're still working to improve the
stability of the implementation. Additionally, Clang may print warnings
that code may have miscompiled. We find this often to be a false alarm
where the wrong interfaces have been used in LLVM and where resulting
code is not actually incorrect. However, this warrants a bug report
and investigation. If you find any bugs or issues, please raise them on
bugs.llvm.org and let us know!

Diff Detail

Event Timeline

sdesmalen created this revision.Jun 12 2020, 2:37 AM
sdesmalen edited the summary of this revision. (Show Details)Jun 23 2020, 12:59 AM
sdesmalen added reviewers: david-arm, SjoerdMeijer.

Hmmm, this is a very big patch because all the tests now need to define these macros, because with %clang_cc1 we don't use the driver. Would it be easier and more consistent with other tests to just use %clang (and don't pass all these defines)?

Hmmm, this is a very big patch because all the tests now need to define these macros, because with %clang_cc1 we don't use the driver. Would it be easier and more consistent with other tests to just use %clang (and don't pass all these defines)?

This patch actually removes all the defines from the tests (those are now implicitly defined when having +sve or +sve2 as the target feature).

Facepalm! That's exactly what I was expecting, but got confused somehow looking at the diffs (this was before my first coffee)!

So yes, this makes sense, one quick question on these macros, are they tested in clang/test/Preprocessor/aarch64-target-features.c, or somewhere else?

Facepalm! That's exactly what I was expecting, but got confused somehow looking at the diffs (this was before my first coffee)!

So yes, this makes sense, one quick question on these macros, are they tested in clang/test/Preprocessor/aarch64-target-features.c, or somewhere else?

The __ARM_FEATURE_SVE macro is indeed checked in aarch64-target-features.c. The SVE2 macros are not because the feature is not yet associated with any architecture version, but the SVE2 tests themselves check that the builtins are guarded by the right macros (which are set when passing target-feature +sve2[+aes|sha3|..])

Facepalm! That's exactly what I was expecting, but got confused somehow looking at the diffs (this was before my first coffee)!

So yes, this makes sense, one quick question on these macros, are they tested in clang/test/Preprocessor/aarch64-target-features.c, or somewhere else?

The __ARM_FEATURE_SVE macro is indeed checked in aarch64-target-features.c. The SVE2 macros are not because the feature is not yet associated with any architecture version, but the SVE2 tests themselves check that the builtins are guarded by the right macros (which are set when passing target-feature +sve2[+aes|sha3|..])

We are defining new macros, e.g. __ARM_FEATURE_SVE2_AES", "1", so I think the only thing we need to add is a CHECK: define __ARM_FEATURE_SVE2_AES 1 to aarch64-target-features.c (and the same for the others).

sdesmalen updated this revision to Diff 272985.Jun 24 2020, 5:10 AM

Added tests for feature macros to aarch64-target-features.c

SjoerdMeijer accepted this revision.Jun 24 2020, 5:44 AM

Cheers, that's it I think, LGTM.

This revision is now accepted and ready to land.Jun 24 2020, 5:44 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2020, 12:30 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript