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

[FMV] Add Priority syntax for version selection order #85

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

[FMV] Add Priority syntax for version selection order #85

Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
564d108
Support version selection
BeMg Aug 6, 2024
249c7fa
are equally -> have equal
BeMg Aug 19, 2024
2b9c6a4
Priority -> priority
BeMg Aug 19, 2024
bfbc793
Make Version Selection as hypelink
BeMg Aug 19, 2024
b96ead9
Update
BeMg Aug 23, 2024
3430d18
Make default doesn't accept other attr
BeMg Aug 23, 2024
07143ea
implementation-specific instead of platform-specific
BeMg Sep 13, 2024
c1d9329
Make rule more flexibility on equal priority
BeMg Sep 13, 2024
a99cb0e
Make BNF change more friendly for review
BeMg Sep 13, 2024
88261f1
Support minus for priority syntax
BeMg Sep 13, 2024
2dda323
Force priority must with arch string
BeMg Sep 13, 2024
e667006
Simplify 'default' in BNF
BeMg Sep 13, 2024
53ac78f
DIGIT -> DIGITS
BeMg Sep 18, 2024
464e6bb
Reuse the same BNF form between target version and target clone
BeMg Sep 18, 2024
cb3e69c
Drop negative symbol from DIGITS
BeMg Oct 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: 55 additions & 7 deletions riscv-c-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,8 +299,22 @@ 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
Copy link

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.

Copy link
Contributor Author

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?

| 'default'
TARGET-CLONES-ATTR-STRING := 'default'
| ATTR-STRINGS

ATTR-STRINGS := ATTR-STRING
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be

ATTR-STRINGS := ATTR-STRING ';' ATTR-STRINGS
                | ATTR-STRING

that more closely matches how EXTENSIONS is defined.

| ';' ATTR-STRINGS

ATTR-STRING := ARCH-ATTR ';' PRIORITY-ATTR
| PRIORITY-ATTR ';' ARCH-ATTR
| ARCH-ATTR

ARCH-ATTR := 'arch=' EXTENSIONS

PRIORITY-ATTR := 'priority=' DIGIT

DIGIT := '-' [0-9]+
| [0-9]+

EXTENSIONS := <EXTENSION> ',' <EXTENSIONS>
| <EXTENSION>
Expand All @@ -319,7 +333,7 @@ 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;
Expand All @@ -331,6 +345,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.
Copy link

Choose a reason for hiding this comment

The 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?

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 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.

Copy link
Contributor

@topperc topperc Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why do we need to allow negative numbers at all?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>")))`
Expand All @@ -342,8 +358,22 @@ 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'
Copy link

Choose a reason for hiding this comment

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

You have this same non-trivial BNF repeated twice. Maybe replace the second with a back reference to the previous? "is the same as described above for target_clones"? (You can also link that text.)

| ATTR-STRINGS

ATTR-STRINGS := ATTR-STRING
| ';' ATTR-STRINGS

ATTR-STRING := ARCH-ATTR ';' PRIORITY-ATTR
| PRIORITY-ATTR ';' ARCH-ATTR
| ARCH-ATTR

ARCH-ATTR := 'arch=' EXTENSIONS

PRIORITY-ATTR := 'priority=' DIGIT

DIGIT := '-' [0-9]+
Copy link

Choose a reason for hiding this comment

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

Naming: DIGITS (with an S), or NUMBER.

| [0-9]+

EXTENSIONS := <EXTENSION> ',' <EXTENSIONS>
| <EXTENSION>
Expand All @@ -362,13 +392,13 @@ 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;
Expand All @@ -386,6 +416,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
Expand Down Expand Up @@ -708,3 +742,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 implementation-specific.
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 one based on an implementation-defined heuristic.
3. If no other suitable versions are found, fall back to the "default" version.