Page MenuHomePhabricator

[clang][SVE] Activate macro `__ARM_FEATURE_SVE_VECTOR_OPERATORS`.
ClosedPublic

Authored by fpetrogalli on Nov 6 2020, 9:24 AM.

Details

Summary

The macro is emitted when wargeting SVE code generation with the additional command line option -msve-vector-bits=<N>.

The behavior implied by the macro is described in sections "3.7.3.3. Behavior specific to SVE vectors" of the SVE ACLE (Version 00bet6) that can be found at https://developer.arm.com/documentation/100987/latest

Diff Detail

Event Timeline

fpetrogalli created this revision.Nov 6 2020, 9:24 AM
fpetrogalli requested review of this revision.Nov 6 2020, 9:24 AM
rengolin accepted this revision.Nov 6 2020, 9:57 AM

Simple and straightforward, with documentation! :)

LGTM, thanks!

This revision is now accepted and ready to land.Nov 6 2020, 9:57 AM

NFC: update tests to use %clang_cc1 instead of %clang to prevent errors for missing includes: https://reviews.llvm.org/B77907

peterwaller-arm added inline comments.Nov 9 2020, 4:18 AM
clang/test/CodeGen/aarch64-sve-acle-__ARM_FEATURE_SVE_VECTOR_OPERATORS-expected-error-on-address.c
6 ↗(On Diff #303805)

Suggestion: Where referencing sections you could include the title "3.7.3.3. Behavior specific to SVE vectors" instead of "3.7.3.3" for example. I think this makes it easier to spot misreferences and gives future souls a better chance to understand the context without indirection.

clang/test/CodeGen/aarch64-sve-acle-__ARM_FEATURE_SVE_PREDICATE_OPERATORS.cpp
19 ↗(On Diff #303805)

Do we need tests for unary operators, assignment and comparison operators?
(Page 28, item 3, subitems, 1, 3, 4)

fpetrogalli edited the summary of this revision. (Show Details)Nov 9 2020, 5:41 AM
fpetrogalli marked an inline comment as done.

I removed the enablement of __ARM_FEATURE_SVE_PREDICATE_OPERATORS as the codegen is incorrect.

I have extended testing of the codegen for __ARM_FEATURE_SVE_VECTOR_OPERATORS.

fpetrogalli retitled this revision from [clang][SVE] Additional macros implied by `-msve-vector-bits=<N>`. to [clang][SVE] Activate macro `__ARM_FEATURE_SVE_VECTOR_OPERATORS`..Nov 9 2020, 9:49 AM
fpetrogalli edited the summary of this revision. (Show Details)
fpetrogalli edited the summary of this revision. (Show Details)

In the last update I removed the tests for __ARM_FEATURE_SVE_PREDICATE_OPERATORSbut forgot to remove the code that generates and tests the macro.

I have added more test coverage for the codegen of the examples mentioned in section 3.7.3.3 of the SVE ACLE. The tests are generic to work for -msve-vector-bits=128|...|2048.

@rengolin - thank you for looking into this. The patch has changed quite a bit since you approved it (faulty codegen for one of the macros, which I have removed from this patch). You might want to re-look at it.

The tests look good to me FWIW. The Sema side is already covered by Sema/attr-arm-sve-vector-bits.c and the patch seems to test for the important ABI bits.

rsandifo-arm accepted this revision.Nov 12 2020, 3:32 AM
rengolin accepted this revision.Nov 13 2020, 2:02 AM

Hi @fpetrogalli, the document is so dense that it took me a while to check the macros and I was still wrong.

Either I'm losing my skill to read Arm documents or folks are getting worse at writing them.

Giving this is a change that only affects SVE targets, and I have no way of independently verify codegen, I'm trusting you and the rest of Arm to make sure it's the right semantics. :)

Thanks!

This revision was landed with ongoing or failed builds.Nov 25 2020, 2:17 AM
This revision was automatically updated to reflect the committed changes.