Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove request for preprocessor guards from header files. #321

Merged
merged 14 commits into from
Aug 14, 2024
Merged

Remove request for preprocessor guards from header files. #321

Changes from 9 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
fc50eb1
Remove request for preprocessor guards from header files.
CarolineConcatto May 29, 2024
994aec6
Re-add the need for __STDC_FORMAT_MACROS
CarolineConcatto May 30, 2024
1270932
Fix unwanted change
CarolineConcatto May 30, 2024
6e3991a
Re-add __ARM_NEON_SVE_BRIDGE it is related to compiler support
CarolineConcatto May 30, 2024
9079875
Adde missing sentence for __ARM_NEON_SVE_BRIDGE
CarolineConcatto May 30, 2024
ae42458
Update acle.md
CarolineConcatto May 30, 2024
92e5b8c
Add more information about the macro headers
CarolineConcatto Jul 2, 2024
fb4b4f8
Add a section in headers to explain headers and macros
CarolineConcatto Jul 8, 2024
15bd063
Re-write section with Predefined macros and header files
CarolineConcatto Aug 2, 2024
4fafea9
Apply suggestions from code review
CarolineConcatto Aug 6, 2024
50f5670
Re-write predefined feature macros and header files section
CarolineConcatto Aug 6, 2024
9d357be
Apply suggestions from code review
CarolineConcatto Aug 7, 2024
cd750bf
Address review comments
CarolineConcatto Aug 7, 2024
7b3ee34
Re-write sentence about the header availability
CarolineConcatto Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 49 additions & 13 deletions main/acle.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -925,8 +925,9 @@ and:
to the more specific header files below. These intrinsics are in the
C implementation namespace and begin with double underscores. It is
unspecified whether they are available without the header being
included. The `__ARM_ACLE` macro should be tested before including the
header:
included. The `__ARM_ACLE` macro can be tested before including the header.
However, this is not recommended for cases where target feature selection is
overridden within code, for example, when using #function-multi-versioning.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
included. The `__ARM_ACLE` macro can be tested before including the header.
However, this is not recommended for cases where target feature selection is
overridden within code, for example, when using #function-multi-versioning.
included. When `__ARM_ACLE` is defined to `1`, the header file is
guaranteed to be available.

When the compiler sets the preprocessor macro to 1, that indicates that it supports the ACLE for the specified feature and that the corresponding header file and intrinsics and types are available. The inverse is not necessarily true, i.e. when the compiler does not define the macro, that doesn't mean that the header file and corresponding intrinsics/types are not available. Compilers that support the target attribute may (and probably should) make it possible to include these header files and make the intrinsics/types available, even when the macro is not set.

However, when a user doesn't test for the macro, it could still be that the header file is not available and the compiler will fail to build the file. For example, if the compiler is too old to recognise <arm_some_feature.h>.

It is probably sufficient to say that the header is guaranteed to be available if the macro is set. (same comment for all the similar cases below)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but also: __ARM_ACLE is a special case in that, like __ARM_NEON_SVE_BRIDGE, it describes the presence of a header file rather than a specific architecture feature. So I think the original text was ok too (and so is your suggestion).

For the feature macros, maybe “is available in a normal unannotated function” would be more specific than “is available”.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this was suggested by @paulwalker-arm

The XYZ macro can be tested before including the header. However, this is not recommended for cases where target feature selection is overridden within code, for example, when using #function-multi-versioning.
From another previous asked change.

I have no preference, but would be nice to agree in what to write here, before I change again. The sentence you see here is a mix of previous interaction that we agree we should replace taking into account FMV.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that wording is appropriate for __ARM_ACLE though (specifically that macro, not others). There is no change in features that would affect whether the compiler provides <arm_acle.h>. Intrinsics within <arm_acle.h> that depend on features have their own __ARM_FEATURE_* macro.


``` c
#ifdef __ARM_ACLE
Expand All @@ -939,8 +940,10 @@ header:
`<arm_fp16.h>` is provided to define the scalar 16-bit floating point
arithmetic intrinsics. As these intrinsics are in the user namespace,
an implementation would not normally define them until the header is
included. The `__ARM_FEATURE_FP16_SCALAR_ARITHMETIC` feature macro
should be tested before including the header:
included. The `__ARM_FEATURE_FP16_SCALAR_ARITHMETIC` macro can be tested before
including the header. However, this is not recommended for cases where target
feature selection is overridden within code, for example, when using
#function-multi-versioning.

``` c
#ifdef __ARM_FEATURE_FP16_SCALAR_ARITHMETIC
Expand All @@ -953,8 +956,10 @@ should be tested before including the header:
`<arm_bf16.h>` is provided to define the 16-bit brain floating point
arithmetic intrinsics. As these intrinsics are in the user namespace,
an implementation would not normally define them until the header is
included. The `__ARM_FEATURE_BF16` feature macro
should be tested before including the header:
included. The `__ARM_FEATURE_BF16` macro can be tested before including the
header. However, this is not recommended for cases where target feature
selection is overridden within code, for example, when using
function-multi-versioning.

``` c
#ifdef __ARM_FEATURE_BF16
Expand All @@ -975,8 +980,11 @@ instructions available are conversion intrinsics between `bfloat16_t` and
intrinsics](#advanced-simd-neon-intrinsics) and associated
[data types](#vector-data-types). As these intrinsics and data types are
in the user namespace, an implementation would not normally define them
until the header is included. The `__ARM_NEON` macro should be tested
before including the header:
until the header is included. The `__ARM_NEON` macro can be tested before
including the header. However, this is not recommended for cases where target
feature selection is overridden within code, for example, when using
#function-multi-versioning.


``` c
#ifdef __ARM_NEON
Expand All @@ -999,8 +1007,9 @@ following it. --><span id="arm_sve.h"></span>
`<arm_sve.h>` defines data types and intrinsics for SVE and its
extensions; see [SVE language extensions and
intrinsics](#sve-language-extensions-and-intrinsics) for details.
You should test the `__ARM_FEATURE_SVE` macro before including the
header:
The `__ARM_FEATURE_SVE` macro can be tested before including the header.
However, this is not recommended for cases where target feature selection is
overridden within code, for example, when using #function-multi-versioning.

``` c
#ifdef __ARM_FEATURE_SVE
Expand All @@ -1019,7 +1028,7 @@ Including `<arm_sve.h>` also includes the following header files:

`<arm_neon_sve_bridge.h>` defines intrinsics for moving data between
Neon and SVE vector types; see [NEON-SVE Bridge](#neon-sve-bridge)
for details. The `__ARM_NEON_SVE_BRIDGE` macro should be tested
for details. The `__ARM_NEON_SVE_BRIDGE` macro should be tested
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: unnecessary change.

before including the header:

``` c
Expand Down Expand Up @@ -1061,8 +1070,10 @@ change or be extended in the future.

`<arm_sme.h>` declares functions and defines intrinsics for SME
and its extensions; see [SME language extensions and intrinsics](#sme-language-extensions-and-intrinsics)
for details. The `__ARM_FEATURE_SME` macro should be tested before
including the header:
for details. The `__ARM_FEATURE_SME` macro can be tested before including the
header. However, this is not recommended for cases where target feature
selection is overridden within code, for example, when using
#function-multi-versioning.

``` c
#ifdef __ARM_FEATURE_SME
Expand All @@ -1072,6 +1083,31 @@ including the header:

Including `<arm_sme.h>` also includes [`<arm_sve.h>`](#arm_sve.h).

### Predefined feature macros and header files

CarolineConcatto marked this conversation as resolved.
Show resolved Hide resolved
Testing a feature macro returns the compiler's availability for the feature via ACLE
documented intrinsics and inline assembly. This property can be independent of
where the test is performed, meaning a feature macro does not guarantee the
validity of using the feature at the point it is tested. For example:
CarolineConcatto marked this conversation as resolved.
Show resolved Hide resolved

``` c
#include <arm_sme.h>

void foo(svbool_t pg, void *ptr, uint32_t slice_base) {
#ifdef __ARM_FEATURE_SME
svst1_hor_za8(0, slice_base, pg, ptr);
CarolineConcatto marked this conversation as resolved.
Show resolved Hide resolved
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the paragraph above is more about the evaluation context/order of the macro to express that a user can't rely on the macro being set/unset when used with the ACLE keyword attributes, e.g.

__attribute__((target("+sve")))
void foo() {
#ifdef __ARM_FEATURE_SVE
  // The user should make no assumptions that the target attribute
  // has enabled the __ARM_FEATURE_SVE macro.
#endif
}

The current example is more about showing that the compiler can add additional restrictions to the use of an intrinsic beyond what is captured by the ACLE macro. If that is the intent of the example, I think it would be helpful if that can be added as an separate paragraph, e.g. something like "The compiler may add additional restrictions to the intrinsics beyond what is captured by the ACLE macros depending on the context in which the intrinsics are used".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @sdesmalen-arm for the suggestions.
I made some changes according to your comments.

}
```

Whilst testing `__ARM_FEATURE_SME` ensures the compiler has available the SME
intrinsic `svst1_hor_za8`, `foo` will fail to compile because it has not been
decorated with the necessary keywords to guarantee the function will be
executed in streaming mode.
CarolineConcatto marked this conversation as resolved.
Show resolved Hide resolved

Not all such issues can be caught during compilation and may instead result in
runtime failures.
CarolineConcatto marked this conversation as resolved.
Show resolved Hide resolved

## Attributes

GCC-style attributes are provided to annotate types, objects and
Expand Down
Loading