-
Notifications
You must be signed in to change notification settings - Fork 42
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
[FMV] Add Priority
syntax for version selection order
#85
base: main
Are you sure you want to change the base?
Changes from 6 commits
564d108
249c7fa
2b9c6a4
bfbc793
b96ead9
3430d18
07143ea
c1d9329
a99cb0e
88261f1
2dda323
e667006
53ac78f
464e6bb
cb3e69c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -299,27 +299,37 @@ Each `TARGET-CLONES-ATTR-STRING` defines a distinguished version of the function | |
The syntax of `<TARGET-CLONES-ATTR-STRING>` describes below: | ||
|
||
``` | ||
TARGET-CLONES-ATTR-STRING := 'arch=' EXTENSIONS | ||
| 'default' | ||
TARGET-CLONES-ATTR-STRING := DEFAULT-ATTR-STRING | ||
| ATTR-STRINGS | ||
|
||
EXTENSIONS := <EXTENSION> ',' <EXTENSIONS> | ||
| <EXTENSION> | ||
ATTR-STRINGS := ATTR-STRING | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be
that more closely matches how EXTENSIONS is defined. |
||
| ';' ATTR-STRINGS | ||
|
||
EXTENSION := <OP> <EXTENSION-NAME> <VERSION> | ||
DEFAULT-ATTR-STRING := 'default' | ||
|
||
OP := '+' | ||
ATTR-STRING := 'arch=' EXTENSIONS | ||
| 'priority=' DIGIT | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is correct. Priority without arch isn't allowed per the english description below. Also, why not priority 15? Or -1? In particular, your LLVM patch appears to accept -1. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Update BNF to let priority only appear when architecture exists.
Update the DIGIT to allow '-'. Does [0-9]+ already support 15? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "DIGIT" is not plural which is what caused my misreading. :) |
||
|
||
VERSION := [0-9]+ 'p' [0-9]+ | ||
| [1-9][0-9]* | ||
| | ||
EXTENSIONS := <EXTENSION> ',' <EXTENSIONS> | ||
| <EXTENSION> | ||
|
||
EXTENSION-NAME := Naming rule is defined in RISC-V ISA manual | ||
EXTENSION := <OP> <EXTENSION-NAME> <VERSION> | ||
|
||
OP := '+' | ||
|
||
VERSION := [0-9]+ 'p' [0-9]+ | ||
| [1-9][0-9]* | ||
| | ||
|
||
DIGIT := [0-9]+ | ||
|
||
EXTENSION-NAME := Naming rule is defined in RISC-V ISA manual | ||
``` | ||
|
||
For example, the following `foo` function will have three versions but share the same function signature. | ||
|
||
```c | ||
__attribute__((target_clones("arch=+v", "default", "arch=+zbb"))) | ||
__attribute__((target_clones("arch=+v;priority=2", "default", "arch=+zbb;priority=1"))) | ||
int foo(int a) | ||
{ | ||
return a + 5; | ||
|
@@ -331,6 +341,8 @@ int bar() { | |
} | ||
``` | ||
|
||
The `priority` accepts a digit as the version priority during [Version Selection](#version-selection). If `priority` isn't specified, then the priority of version defaults to zero. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that we accept -1, we have to state ordering. Does "-1" compare less than "1" (i.e. signed ordering), or does it mean "maximum priority" (i.e. unsigned ordering)? I think we probably want unsigned here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume the signed ordering here, and I also use it in the LLVM implementation. It's just more intuitive to me, but I can change to unsigned ordering if necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to allow negative numbers at all? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @preames do you see any reason to support negative numbers here? The number of target_clones/target_version attributes should be small enough that setting even a smallish number like 1000 should be high enough to be maximum priority. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, restricting it to positive only would be fine. I just want to make sure the interpretation is unambiguous. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing negative numbers from the priority appears to be a clearer solution to avoid ambiguity. I will update this in the pull request. |
||
|
||
It makes the compiler trigger the [function multi-version](#function-multi-version) when there exist more than one version for the same function signature. | ||
|
||
### `__attribute__((target_version("<TARGET-VERSION-ATTR-STRING>")))` | ||
|
@@ -342,33 +354,43 @@ Each `TARGET-VERSION-ATTR-STRING` defines a distinguished version of the functio | |
The syntax of `<TARGET-VERSION-ATTR-STRING>` describes below: | ||
|
||
``` | ||
TARGET-VERSION-ATTR-STRING := 'arch=' EXTENSIONS | ||
| 'default' | ||
TARGET-VERSION-ATTR-STRING := DEFAULT-ATTR-STRING | ||
| ATTR-STRINGS | ||
|
||
EXTENSIONS := <EXTENSION> ',' <EXTENSIONS> | ||
| <EXTENSION> | ||
DEFAULT-ATTR-STRING := 'default' | ||
|
||
EXTENSION := <OP> <EXTENSION-NAME> <VERSION> | ||
ATTR-STRINGS := ATTR-STRING | ||
| ';' ATTR-STRINGS | ||
|
||
OP := '+' | ||
ATTR-STRING := 'arch=' EXTENSIONS | ||
| 'priority=' DIGIT | ||
|
||
VERSION := [0-9]+ 'p' [0-9]+ | ||
| [1-9][0-9]* | ||
| | ||
EXTENSIONS := <EXTENSION> ',' <EXTENSIONS> | ||
| <EXTENSION> | ||
|
||
EXTENSION-NAME := Naming rule is defined in RISC-V ISA manual | ||
EXTENSION := <OP> <EXTENSION-NAME> <VERSION> | ||
|
||
OP := '+' | ||
|
||
VERSION := [0-9]+ 'p' [0-9]+ | ||
| [1-9][0-9]* | ||
| | ||
|
||
DIGIT := [0-9]+ | ||
|
||
EXTENSION-NAME := Naming rule is defined in RISC-V ISA manual | ||
``` | ||
|
||
For example, the following foo function has three versions. | ||
|
||
```c | ||
__attribute__((target_version("arch=+v"))) | ||
__attribute__((target_version("arch=+v;priority=1"))) | ||
int foo(int a) | ||
{ | ||
return a + 5; | ||
} | ||
|
||
__attribute__((target_version("arch=+zbb"))) | ||
__attribute__((target_version("arch=+zbb;priority=2"))) | ||
int foo(int a) | ||
{ | ||
return a + 5; | ||
|
@@ -386,6 +408,10 @@ int bar() { | |
} | ||
``` | ||
|
||
The `priority` accepts a digit as the version priority during [Version Selection](#version-selection). If `priority` isn't specified, then the priority of version defaults to zero. | ||
|
||
The `default` version does not accept the priority. | ||
|
||
It makes the compiler trigger the [function multi-version](#function-multi-version) when there exist more than one version for the same function signature. | ||
|
||
### riscv_vector_cc | ||
|
@@ -708,3 +734,17 @@ statements, including both RISC-V specific and common operand modifiers. | |
Function multi-versioning(FMV) provides an approach to selecting the appropriate function according to the runtime environment. The final binary may contain all versions of the function, with the compiler generating all supported versions and the runtime selecting the appropriate one. | ||
|
||
This feature is triggered by `target_version/target_clones` function attribute. | ||
|
||
### Version Selection | ||
|
||
The process of selecting the appropriate function version during function multi-versioning follows these guidelines: | ||
|
||
1. The implementation of the selection algorithm is platform-specific. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please change "platform-specific" to "implementation-specific". This allows e.g. different toolchains to have different tie breakers while still following the priority rules. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. |
||
2. Once a version is selected, it remains in use for the entire duration of the process. | ||
3. Only versions whose required features are all available in the runtime environment are eligible for selection. | ||
|
||
The version selection process applies the following rules in order: | ||
|
||
1. Among the eligible versions, select the one with the highest priority. | ||
2. If multiple versions have equal priority, select the one that was declared first. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to mandate (2)? We can leave ourselves more flexibility by saying "select one based on an implementation defined heuristic". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's a good idea to maintain more flexibility. Updated. Thanks! |
||
3. If no other suitable versions are found, fall back to the "default" version. |
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.
It looks like you may have re-odered the BPF. Please undo this as it makes the diff really hard to read.
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.
Does it look better now?