-
Notifications
You must be signed in to change notification settings - Fork 54
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.
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
Remove request for preprocessor guards from header files. #321
Conversation
…#95224) To enable function multi-versioning (FMV), current checks which rely on cmd line options or global macros to see if target feature is present need to be removed. This patch removes those for NEON and also implements changes to NEON header file as proposed in [ACLE](ARM-software/acle#321).
main/acle.md
Outdated
`__ARM_ACLE`, `__ARM_FEATURE_FP16_SCALAR_ARITHMETIC`, `__ARM_FEATURE_BF16`, | ||
`__ARM_NEON`, `__ARM_FEATURE_SVE` and ` __ARM_FEATURE_SME` | ||
represent a compiler's ability to use the instructions associated | ||
with the feature via ACLE documented builtins and within inline assembly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to use "documented intrinsics", the fact that they are implemented as builtins is an implantation detail.
main/acle.md
Outdated
@@ -1072,6 +1074,33 @@ including the header: | |||
|
|||
Including `<arm_sme.h>` also includes [`<arm_sve.h>`](#arm_sve.h). | |||
|
|||
These macro features: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph is added in a strange place, to me looks like it will fall int the section about arm_sme, while this is not strictly related to sme, maybe better to move it to the beginning, I mean the paragraphs about "Header files"?
IMO you can be more general and do not have to list all of the feature macros.
main/acle.md
Outdated
represent a compiler's ability to use the instructions associated | ||
with the feature via ACLE documented builtins and within inline assembly. | ||
They may also be influenced by source code mechanisms like `#pragma` or | ||
`attribute`, but users should not rely on this. They are defined for user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is badly phrased. Do you mean here that users should not rely on #pragma or attribute or the feature.
macros?
If it is about the macros, I think we need to phrase it differently and explain that the presence of the macro does not guarantee that the architecture feature is always present at various stages of compilation or execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Maciej,
I re-wrote this paragraph and added a section at the end of HEADER to explain headers + macros.
Just in case the ACLE has a section: Feature test macros. It is generic, it is about all predefined macros.
The point here is that the ACLE/I want to explain that headers protected by macro does not imply we can always use intrinsics.
…llvm#95224) To enable function multi-versioning (FMV), current checks which rely on cmd line options or global macros to see if target feature is present need to be removed. This patch removes those for NEON and also implements changes to NEON header file as proposed in [ACLE](ARM-software/acle#321).
main/acle.md
Outdated
included. The `__ARM_ACLE` macro can be tested before including the | ||
header, but this might limit function multi-versioning capabilities: |
There was a problem hiding this comment.
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.
main/acle.md
Outdated
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. | ||
|
||
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. | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
main/acle.md
Outdated
@@ -1072,6 +1074,32 @@ including the header: | |||
|
|||
Including `<arm_sme.h>` also includes [`<arm_sve.h>`](#arm_sve.h). | |||
|
|||
### Predefined Macros and Headers |
There was a problem hiding this comment.
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" ?
Thank you @paulwalker-arm, @mgabka and @rsandifo-arm for your inputs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Let's wait until the end of this week for additional feedback |
main/acle.md
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
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”.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
main/acle.md
Outdated
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. |
There was a problem hiding this comment.
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”.
Co-authored-by: rsandifo-arm <[email protected]>
main/acle.md
Outdated
#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 |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
main/acle.md
Outdated
included. When `__ARM_FEATURE_FP16_SCALAR_ARITHMETIC` is defined to `1`, | ||
the header file is available in a normal unannotated function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The macro is also available outside of function scope. The scope in which it is used shouldn't matter, because it is a preprocessor macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote again this sentence to:
When __ARM_xxx
is defined to 1
, the header file is available
regardless of the context in which the macro is evaluated.
Co-authored-by: Sander de Smalen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm content with the phrasing now. Please address the nits though.
@@ -1019,7 +1023,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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: unnecessary change.
// The user should make no assumptions that the target attribute | ||
// has enabled the __ARM_FEATURE_SVE macro. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: whitespace
// The user should make no assumptions that the target attribute | |
// has enabled the __ARM_FEATURE_SVE macro. | |
// The user should make no assumptions that the target attribute | |
// has enabled the __ARM_FEATURE_SVE macro. |
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. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not convinced this is worth pointing out, because the compiler may also enforce requirements on the range of immediate arguments, and we never bothered to document that either. That said though, I don't see any harm in adding this either.
Since this PR got approval with minor nits, I'm merging it. @CarolineConcatto please address the remaining nits in a following PR. Thanks |
} | ||
``` | ||
|
||
The compiler may add additional restrictions to the intrinsics beyond what is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
``` | ||
|
||
If `__ARM_FEATURE_SME` evaluates to `true` the SME intrinsic `svst1_hor_za8` | ||
is available, but `foo` may still fail to compile because the call does not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor changes
name: Pull request
about: Technical issues, document format problems, bugs in scripts or feature proposal.
Thank you for submitting a pull request!
If this PR is about a bugfix:
Please use the bugfix label and make sure to go through the checklist below.
If this PR is about a proposal:
We are looking forward to evaluate your proposal, and if possible to
make it part of the Arm C Language Extension (ACLE) specifications.
We would like to encourage you reading through the contribution
guidelines, in particular the section on submitting
a proposal.
Please use the proposal label.
As for any pull request, please make sure to go through the below
checklist.
Checklist: (mark with
X
those which apply)PR (do not bother creating the issue if all you want to do is
fixing the bug yourself).
SPDX-FileCopyrightText
lines on topof any file I have edited. Format is
SPDX-FileCopyrightText: Copyright {year} {entity or name} <{contact informations}>
(Please update existing copyright lines if applicable. You can
specify year ranges with hyphen , as in
2017-2019
, and usecommas to separate gaps, as in
2018-2020, 2022
).Copyright
section of the sources of thespecification I have edited (this will show up in the text
rendered in the PDF and other output format supported). The
format is the same described in the previous item.
tricky to set up on non-*nix machines). The sequence can be
found in the contribution
guidelines. Don't
worry if you cannot run these scripts on your machine, your
patch will be automatically checked in the Actions of the pull
request.
introduced in this PR in the section Changes for next
release of the section Change Control/Document history
of the document. Create Changes for next release if it does
not exist. Notice that changes that are not modifying the
content and rendering of the specifications (both HTML and PDF)
do not need to be listed.
correctness of the result in the PDF output (please refer to the
instructions on how to build the PDFs
locally).
draftversion
is set totrue
in the YAML headerof the sources of the specifications I have modified.
in the README page of the project.