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 8 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
54 changes: 41 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,8 @@ 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, but this might limit function multi-versioning capabilities:
Copy link

Choose a reason for hiding this comment

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

Ignore me if this format has already been agreed but if not can I recommend:

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.


``` c
#ifdef __ARM_ACLE
Expand All @@ -939,8 +939,9 @@ 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, but this might limit function
multi-versioning capabilities:

``` c
#ifdef __ARM_FEATURE_FP16_SCALAR_ARITHMETIC
Expand All @@ -953,8 +954,8 @@ 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, but this might limit function multi-versioning capabilities:

``` c
#ifdef __ARM_FEATURE_BF16
Expand All @@ -975,8 +976,9 @@ 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, but this might limit function multi-versioning
capabilities:

``` c
#ifdef __ARM_NEON
Expand All @@ -999,8 +1001,8 @@ 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,
but this might limit function multi-versioning capabilities:

``` c
#ifdef __ARM_FEATURE_SVE
Expand All @@ -1019,7 +1021,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 +1063,8 @@ 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, but this might limit function multi-versioning capabilities:

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

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

### Predefined Macros and Headers
Copy link

Choose a reason for hiding this comment

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

should this be changed to : "### predefined feature macros and header files" ?


CarolineConcatto marked this conversation as resolved.
Show resolved Hide resolved
The macros can represent a compiler's ability to use the instructions
associated with the feature via ACLE documented intrinsics and within inline
assembly. However users should not rely solely on macro definition, because
there are intrinsics that may be influenced by mechanisms like `#pragma` or
`attribute`. The macros are defined only for user convenience.

CarolineConcatto marked this conversation as resolved.
Show resolved Hide resolved
For example:

``` c
#ifdef __ARM_FEATURE_SME
#include<arm_sme.h>
void foo(svbool_t pg, void *ptr, uint32_t slice_base) {
svst1_hor_za8(0, slice_base, pg, ptr);
}
#endif
```

`foo` uses `__ARM_FEATURE_SME` to have available SME intriniscs,
like `svst1_hor_za8`. However it has the compilation error:
`builtin can only be called from a streaming function`
for the command line:
`armclang -O3 -target aarch64-linux-gnu -march=armv8-a+sme`
that defines `__ARM_FEATURE_SME` to one.

Copy link

Choose a reason for hiding this comment

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

How about?

Testing a feature macro returns the compiler's support 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:

    #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);
    #endif
    }

Whilst testing `__ARM_FEATURE_SME` ensures the compiler supports 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.

Not all such issues can be caught during compilation and may instead result in runtime failures.

Copy link

Choose a reason for hiding this comment

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

Testing a feature macro returns the compiler's support for the feature via ACLE documented intrinsics and inline assembly.

Not sure if it is just me, the key is how we define "support".
For me compiler supports ACLE when the given ACLE is implemented. While the feature macro is only defined when appropriate features are enabled in the cmd line option only. Maybe worth to change it to sth like:

"Testing a feature macro allows to check if the compiler can make use of the architecture feature via ACLE documented intrinsics and inline assembly assuming the provided compilation flags."

What do you think?

Copy link

Choose a reason for hiding this comment

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

This distinction is not relevant because there is no assumption about compilation flags. The macro alone is sufficient to detect at the source level whether the compiler (in the mode it's in when compiling the input) can support a specific feature. By support we effectively mean "is able to compile the input when it is correctly formed".

The provided example shows something that is not "correctly formed" where the compiler will report an error that must be fixed before the code will compile. There are many other examples but I don't see a reason to document them all here.

Note I say "compile" and not "compile to a working binary" because the more typical result is the compiler correctly building the input which then generates a SIGILL when executed. This is covered by the "Not all such issues..." sentence. Personally I don't think it's worth going into anymore detail than this.

Copy link

Choose a reason for hiding this comment

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

Sure, I get the difference between "compile" and not "compile to a working binary".

For me it was just the meaning of word support, as in my mind feature could be supported (as implemented) by a tool but not available when given set of options it provided. It has this small difference from the user point of view as if feature is not implemented they can not do anything, while if it is not enabled they can change options.
But I am not against your version either.

Copy link

Choose a reason for hiding this comment

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

I think that's because you are considering this from the point of view of a human asking whether the compiler can support a feature. Whereas the feature macros are source code actions and thus the context is the point the source code is being compiled, at which point the compiler has already been "configured" (for want of a better word) by the user.

Perhaps the point you are raising is the document does not detail what actions the user should take in order to control the definitions of the feature flags, which is intentional because we cannot dictate that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it would be good to be a bit more precise about what “support” means though. At one level, GCC 14 “supports” SME2, and the word is often used in that sense. So if we just say “support”, users might expect GCC 14 to define __ARM_FEATURE_SME2 unconditionally.

Instead, for compatibility, we have to define the macro only if the intrinsics are “available” in functions that have no target annotations (not precise wording either). What users need to do to achieve that for a given compiler is outside the scope of the ACLE, but I think the result is within scope.

IMO, the example of using SME intrinsics in non-streaming functions is an example of an intrinsics being available, but being used incorrectly. It's somewhat like passing an out-of-range value to a constant argument, except that the error is contextual, rather than contained within the call. (I still think it's a good example though.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did changed to what @paulwalker-arm requested, but instead of using support I wrote available, as suggested by @rsandifo-arm.
I hope it is correct.

## Attributes

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