-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add clarification and restrictions for FMV #363
base: main
Are you sure you want to change the base?
Conversation
This clarifies cases like: ```C __attribute__((target_version("default"))) int foo(int (* x)[]){ return (*x)[11]; } __attribute__((target_version("sve2"))) int foo(int (* x)[12]){ return (*x)[11]; } __attribute__((target_version("sve"))) int foo(int (* x)[10]){ // Error due to conflicting signatures return (*x)[7]; } int bar() { int y[10]; return foo(&y); } ``` ```C int __attribute__((target_version("default"))) fn (int x) { return 1; } void bar () { int __attribute__((target_version("sve2"))) fn (int); // Error due // to non file scope declaration fn(1); } ``` ```C __attribute__((target_version("sve"))) int foo(){ return 2; } __attribute__((target_version("sve2"))) int foo(){ return 2; } int bar() { return foo(); // Error, due to no visability of default version } ``` and ```C // Translation unit 1: __attribute__((target_version("default"))) int foo(); __attribute__((target_version("simd"))) int foo(){ return 2; } __attribute__((target_version("sve"))) int bar() { return foo(); // Should NOT be optimized to use the simd version // even though it is the most specific version in this translation // unit as this would lead to different versions being used within // the same project } // Translation unit 2: __attribute__((target_version("default"))) int foo() { return 1; }; __attribute__((target_version("simd"))) int foo(); __attribute__((target_version("sve"))) int foo(){ return 3; } ```
@@ -2696,10 +2701,12 @@ following: | |||
in one of the translation units. | |||
* Implicitly, without this attribute, | |||
* or explicitly providing the `default` in the attribute. | |||
* All instances of the versions shall share the same function | |||
signature and calling convention. |
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 ACLE is clear on this one. I find the new text confusing. In your example the functions are not using the same signature (the arguments to each version of foo have different array length). There's nothing to be added here in my opinion.
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 the wording should be improved, but I believe there is a need for an adjustment here.
This was specifically something requested by upstream C maintainers in GCC.
The justification is that we likely don't want to require exactly the same signatures, as it
is common for declarations and implementations to differ in signature while still being compatible:
Something like
int __attribute__((target_version("default"))) fn (int x[]);
int __attribute__((target_version("default"))) fn (int x[5]) {
return 1;
}
Or:
int __attribute__((target_version("default"))) fn (bool x = true);
int __attribute__((target_version("default"))) fn (bool x) {
return 1;
}
For a more CPP example.
(LLVM is happy with both these currently)
But if this is allowed at some level then we need to define if information for one version should be shared with others, eg a case like:
int __attribute__((target_version("default"))) fn (bool c = true);
int __attribute__((target_version("sve"))) fn (bool c);
int __attribute__((target_version("default"))) fn (bool c) {
return 1;
}
int bar() {
fn();
}
(Again allowed by LLVM)
Should this default to true for the SVE version? I would assume so but its not stipulated?
We could disallow it completely, but that seems overly restrictive and counter to C and C++ intuitions.
@@ -2682,6 +2683,10 @@ The following attributes trigger the multi version code generation: | |||
* If only the `default` version exist it should be linked directly. | |||
* FMV may be disabled in compile time by a compiler flag. In this | |||
case the `default` version shall be used. | |||
* Scope for a group of multiversioned functions is the scope of the default |
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.
What do you mean here? The example https://godbolt.org/z/qW7hx58bf works fine in LLVM. ACLE mandates:
All the function versions must be declared at the translation unit in which the definition of the default version resides.
This requirement guarantees a single resolver across multiple translation units (see llvm/llvm-project#97761).
We want this example to compile in the absence of default declaration (motivation here llvm/llvm-project#84405 (comment)).
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 completely agree we want to be able to compile in absence of default declaration. I wrote this to exclude the calling or referring to the dispatched function without the default in scope (rather than the declaration).
My model for this is that the default function is the function, and the extra versions amend that function with different versions for different architecture.
This way, if you were to remove all the other version implementations and declarations the code base would still compile.
I think a case like https://godbolt.org/z/qW7hx58bf is emblematic of this point. In this TU there is no indication there is a default version, I think this should require a default version to be declared to be able to call it.
I don't think a versioned function on its own should imply the existence of something callable.
Even in the case you cited (llvm/llvm-project#84405 (comment)) they include the declaration of the default version in all TU's, I believe because it is the idiomatic way to do it?
* All the function versions must be declared at the translation | ||
unit in which the definition of the default version resides. | ||
* All function declarations must be at file scope level. |
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 example https://godbolt.org/z/9c3rP87o9 seems to work with LLVM if you enclose the declarations to a namespace.
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.
Hmm namespaces are a good point (I was looking at this from the C point of view) and hadn't considered it.
I specifically wanted to exclude the case with the versioned function declaration in a function body.
I think it creates the impression that a call in the function will be able to access different versions that outside it.
Maybe it suffices to say all function declarations must be global symbols?
Even in the example you gave, the resolver includes the sve2
dispatched version of the function, but it isn't visible to the default version so surely by ACLE specifications it shouldn't? But what should it do? There seems to then never be any use for a declaration like this so I feel we should disallow it and have an error message?
This clarifies cases like:
and
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.