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

BeMg · 2024-08-07T14:53:20Z

Function multi-versioning provides several versions to choose from. When selecting the appropriate version, there are two factors that impact this process. First is whether a version is eligible. Second is the order in which all eligible versions are tried. For other targets (like aarch64), a priority number is defined for each feature. In this PR, we propose new syntax that allows users to define their own priority for specific versions.

The version selection process applies the following rules in order:

Among the eligible versions, select the one with the highest priority.
If multiple versions have equal priority, select the one that was declared first.
If no other suitable versions are found, fall back to the "default" version.

BeMg · 2024-08-07T14:57:17Z

For example:

__attribute__((target_clones("default", "arch=+zba", "arch=+zbb", "arch=+zba,+zbb")))

The version arch=+zba,+zbb will never be selected in any environment because other eligible versions will always be chosen first.

BeMg · 2024-08-07T14:58:17Z

cc @kito-cheng @topperc

topperc · 2024-08-08T04:05:07Z

riscv-c-api.md

+
+ATTR-STRING            := 'arch=' EXTENSIONS
+                        | 'default'
+                        | 'Priority=' DIGIT 


Why is Priority capitalized when arch isn't?

topperc · 2024-08-08T04:06:24Z

riscv-c-api.md

+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 are equally priority, select the one that was declared first.


"are equally" -> "have equal"

riscv-c-api.md

topperc · 2024-08-22T17:15:25Z

cc: @preames

riscv-c-api.md

BeMg · 2024-08-23T06:29:31Z

Note: Update the BNF to make default version doesn't accept the other attr-string.

preames

Generally supportive of this syntax proposal, subject to inline comments being addressed.

preames · 2024-09-12T18:34:51Z

riscv-c-api.md

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


Please change "platform-specific" to "implementation-specific". This allows e.g. different toolchains to have different tie breakers while still following the priority rules.

preames · 2024-09-12T18:36:19Z

riscv-c-api.md

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


Do we want to mandate (2)? We can leave ourselves more flexibility by saying "select one based on an implementation defined heuristic".

I think it's a good idea to maintain more flexibility. Updated. Thanks!

preames · 2024-09-12T18:37:18Z

riscv-c-api.md

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


It looks like you may have re-odered the BPF. Please undo this as it makes the diff really hard to read.

Does it look better now?

preames · 2024-09-12T18:38:59Z

riscv-c-api.md


-OP                     := '+'
+ATTR-STRING               := 'arch=' EXTENSIONS
+                           | 'priority=' DIGIT 


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.

I don't think this is correct. Priority without arch isn't allowed per the english description below.

Update BNF to let priority only appear when architecture exists.

Also, why not priority 15? Or -1? In particular, your LLVM patch appears to accept -1.

Update the DIGIT to allow '-'. Does [0-9]+ already support 15?

"DIGIT" is not plural which is what caused my misreading. :)

This patch enable the function multiversion(FMV) and `target_clones` attribute for RISC-V target. The proposal of `target_clones` syntax can be found at the riscv-non-isa/riscv-c-api-doc#48 (which has landed), as modified by the proposed riscv-non-isa/riscv-c-api-doc#85 (which adds the priority syntax). It supports the `target_clones` function attribute and function multiversioning feature for RISC-V target. It will generate the ifunc resolver function for the function that declared with target_clones attribute. The resolver function will check the version support by runtime object `__riscv_feature_bits`. For example: ``` __attribute__((target_clones("default", "arch=+ver1", "arch=+ver2"))) int bar() { return 1; } ``` the corresponding resolver will be like: ``` bar.resolver() { __init_riscv_feature_bits(); // Check arch=+ver1 if ((__riscv_feature_bits.features[0] & BITMASK_OF_VERSION1) == BITMASK_OF_VERSION1) { return bar.arch=+ver1; } else { // Check arch=+ver2 if ((__riscv_feature_bits.features[0] & BITMASK_OF_VERSION2) == BITMASK_OF_VERSION2) { return bar.arch=+ver2; } else { // Default return bar.default; } } } ```

preames · 2024-09-13T15:03:54Z

riscv-c-api.md

+
+PRIORITY-ATTR              := 'priority=' DIGIT
+
+DIGIT                     := '-' [0-9]+


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

preames · 2024-09-13T15:06:55Z

riscv-c-api.md

@@ -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'


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

preames · 2024-09-13T15:09:50Z

riscv-c-api.md

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


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?

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.

Why do we need to allow negative numbers at all?

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

Nope, restricting it to positive only would be fine. I just want to make sure the interpretation is unambiguous.

Removing negative numbers from the priority appears to be a clearer solution to avoid ambiguity. I will update this in the pull request.

This patch enable `target_version` attribute for RISC-V target. The proposal of `target_version` syntax can be found at the riscv-non-isa/riscv-c-api-doc#48 (which has landed), as modified by the proposed riscv-non-isa/riscv-c-api-doc#85 (which adds the priority syntax). `target_version` attribute will trigger the function multi-versioning feature and act like `target_clones` attribute. See #85786 for the implementation of `target_clones`.

This patch enable `target_version` attribute for RISC-V target. The proposal of `target_version` syntax can be found at the riscv-non-isa/riscv-c-api-doc#48 (which has landed), as modified by the proposed riscv-non-isa/riscv-c-api-doc#85 (which adds the priority syntax). `target_version` attribute will trigger the function multi-versioning feature and act like `target_clones` attribute. See llvm#85786 for the implementation of `target_clones`.

topperc · 2024-10-07T17:59:08Z

riscv-c-api.md

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


Should this be

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

that more closely matches how EXTENSIONS is defined.

Fix the buildbot failure caused by heap use-after-free error. Origin message: This patch enable `target_version` attribute for RISC-V target. The proposal of `target_version` syntax can be found at the riscv-non-isa/riscv-c-api-doc#48 (which has landed), as modified by the proposed riscv-non-isa/riscv-c-api-doc#85 (which adds the priority syntax). `target_version` attribute will trigger the function multi-versioning feature and act like `target_clones` attribute. See #85786 for the implementation of `target_clones`.

This patch adds support for function multi-versioning to the RISC-V using the target_clones and target_versions attributes, which follow the RISC-V C-API Docs [1] and the existing proposal about priority syntax [2]. This patch copies many codes from commit 0cfde68 ("[aarch64] Add function multiversioning support") and modifies them to fit the RISC-V port. Some key differences are introduced in previously submitted patches [3] and [4], commit [5] and [6]. To test this patch with the GLIBC dynamic loader, you should apply patch [7] for GLIBC to ensure the dynamic loader will initialize the gp register correctly. [1] https://github.com/riscv-non-isa/riscv-c-api-doc/blob/c6c5d6d9cf96b342293315a5dff3d25e96ef8191/src/c-api.adoc#__attribute__targetattr-string [2] riscv-non-isa/riscv-c-api-doc#85 [3] https://patchwork.sourceware.org/project/gcc/patch/[email protected]/ [4] https://patchwork.sourceware.org/project/gcc/patch/[email protected]/ [5] 6183523 [6] 30e0118 [7] https://patchwork.sourceware.org/project/glibc/patch/[email protected]/ Co-Developed-by: Hank Chang <[email protected]> gcc/ChangeLog: * common/config/riscv/riscv-common.cc (struct riscv_ext_bitmask_table_t): New struct. (riscv_minimal_hwprobe_feature_bits): New function. * config/riscv/riscv-protos.h (riscv_option_valid_version_attribute_p): New function. (riscv_process_target_attr): New function. * config/riscv/riscv-subset.h (riscv_minimal_hwprobe_feature_bits): New function. * config/riscv/riscv-target-attr.cc (riscv_target_attr_parser::handle_priority): New function. (riscv_target_attr_parser::update_settings): Update priority attribute and never free the arch string. (riscv_process_one_target_attr): Add const qualifier to arg_str and split arg_str with ';'. (riscv_process_target_attr): New implementation which consumes the const char *args instead of tree. (riscv_option_valid_attribute_p): Reapply any target_version attribute after target attribute. (riscv_process_target_version_attr): New function. * config/riscv/riscv.cc (riscv_can_inline_p): Refuse to inline when callee is versioned but caller is not. (parse_features_for_version): New function. (compare_fmv_features): New function. (riscv_compare_version_priority): New function. (riscv_common_function_versions): New function. (add_condition_to_bb): New function. (dispatch_function_versions): New function. (get_suffixed_assembler_name): New function. (make_resolver_func): New function. (riscv_mangle_decl_assembler_name): New function. (riscv_generate_version_dispatcher_body): New function. (riscv_get_function_versions_dispatcher): New function. (TARGET_OPTION_VALID_VERSION_ATTRIBUTE_P): Implement it. (TARGET_OPTION_FUNCTION_VERSIONS): Implement it. (TARGET_COMPARE_VERSION_PRIORITY): Implement it. (TARGET_GENERATE_VERSION_DISPATCHER_BODY): Implement it. (TARGET_GET_FUNCTION_VERSIONS_DISPATCHER): Implement it. (TARGET_MANGLE_DECL_ASSEMBLER_NAME): Implement it. * config/riscv/riscv.opt: Add TargetVariable riscv_fmv_priority. * defaults.h (TARGET_CLONES_ATTR_SEPARATOR): Define new macro. * multiple_target.cc (get_attr_str): Use TARGET_CLONES_ATTR_SEPARATOR to separate attributes. (separate_attrs): Likewise. * config/riscv/riscv.h (TARGET_CLONES_ATTR_SEPARATOR): Implement it. (TARGET_HAS_FMV_TARGET_ATTRIBUTE): Implement it. * config/riscv/feature_bits.h: New file.

Ensure that target_version and target_clones do not accept negative numbers for the priority feature. Base on discussion on riscv-non-isa/riscv-c-api-doc#85.

This patch adds the priority syntax parser to support the Function Multi-Versioning (FMV) feature in RISC-V. This feature allows users to specify the priority of the function version in the attribute syntax. Chnages based on RISC-V C-API PR: riscv-non-isa/riscv-c-api-doc#85 gcc/ChangeLog: * config/riscv/riscv-target-attr.cc (riscv_target_attr_parser::handle_priority): New function. (riscv_target_attr_parser::update_settings): Update priority attribute. (riscv_process_one_target_attr): Add const qualifier to arg_str and split arg_str with ';'. * config/riscv/riscv.opt: Add TargetVariable riscv_fmv_priority.

Ensure that target_version and target_clones do not accept negative numbers for the priority feature. Base on discussion on riscv-non-isa/riscv-c-api-doc#85.

This patch adds the priority syntax parser to support the Function Multi-Versioning (FMV) feature in RISC-V. This feature allows users to specify the priority of the function version in the attribute syntax. Chnages based on RISC-V C-API PR: riscv-non-isa/riscv-c-api-doc#85 gcc/ChangeLog: * config/riscv/riscv-target-attr.cc (riscv_target_attr_parser::handle_priority): New function. (riscv_target_attr_parser::update_settings): Update priority attribute. (riscv_process_one_target_attr): Add const qualifier to arg_str and split arg_str with ';'. * config/riscv/riscv.opt: Add TargetVariable riscv_fmv_priority.

This patch adds the priority syntax parser to support the Function Multi-Versioning (FMV) feature in RISC-V. This feature allows users to specify the priority of the function version in the attribute syntax. Chnages based on RISC-V C-API PR: riscv-non-isa/riscv-c-api-doc#85 Signed-off-by: Yangyu Chen <[email protected]> gcc/ChangeLog: * config/riscv/riscv-target-attr.cc (riscv_target_attr_parser::handle_priority): New function. (riscv_target_attr_parser::update_settings): Update priority attribute. * config/riscv/riscv.opt: Add TargetVariable riscv_fmv_priority.

Support version selection

564d108

BeMg mentioned this pull request Aug 7, 2024

[RISCV][FMV] Support target_clones llvm/llvm-project#85786

Merged

topperc reviewed Aug 14, 2024

View reviewed changes

BeMg added 2 commits August 19, 2024 02:42

are equally -> have equal

249c7fa

Priority -> priority

2b9c6a4

BeMg force-pushed the FMV-version-order branch from 5c8f451 to df14458 Compare August 19, 2024 10:25

Make Version Selection as hypelink

bfbc793

BeMg force-pushed the FMV-version-order branch from df14458 to bfbc793 Compare August 19, 2024 10:26

topperc reviewed Aug 22, 2024

View reviewed changes

riscv-c-api.md Outdated Show resolved Hide resolved

riscv-c-api.md Outdated Show resolved Hide resolved

topperc reviewed Aug 22, 2024

View reviewed changes

riscv-c-api.md Outdated Show resolved Hide resolved

BeMg added 2 commits August 22, 2024 23:04

Update

b96ead9

Make default doesn't accept other attr

3430d18

preames reviewed Sep 12, 2024

View reviewed changes

preames mentioned this pull request Sep 12, 2024

Function multi-version proposal #48

Merged

BeMg added 6 commits September 12, 2024 21:28

implementation-specific instead of platform-specific

07143ea

Make rule more flexibility on equal priority

c1d9329

Make BNF change more friendly for review

a99cb0e

Support minus for priority syntax

88261f1

Force priority must with arch string

2dda323

Simplify 'default' in BNF

e667006

preames reviewed Sep 13, 2024

View reviewed changes

preames mentioned this pull request Sep 13, 2024

[RISCV][FMV] Support target_version llvm/llvm-project#99040

Merged

BeMg added 2 commits September 17, 2024 20:46

DIGIT -> DIGITS

53ac78f

Reuse the same BNF form between target version and target clone

464e6bb

Drop negative symbol from DIGITS

cb3e69c

topperc reviewed Oct 7, 2024

View reviewed changes

BeMg mentioned this pull request Oct 14, 2024

[RISCV][FMV] Remove support for negative priority llvm/llvm-project#112161

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

BeMg commented Aug 7, 2024

BeMg commented Aug 7, 2024

BeMg commented Aug 7, 2024

topperc Aug 8, 2024

topperc Aug 8, 2024

topperc commented Aug 22, 2024

BeMg commented Aug 23, 2024

preames left a comment

preames Sep 12, 2024

BeMg Sep 13, 2024

preames Sep 12, 2024

BeMg Sep 13, 2024

preames Sep 12, 2024

BeMg Sep 13, 2024

preames Sep 12, 2024

BeMg Sep 13, 2024

preames Sep 13, 2024

preames Sep 13, 2024

preames Sep 13, 2024

preames Sep 13, 2024

BeMg Sep 25, 2024

topperc Sep 25, 2024 •

edited

Loading

topperc Oct 2, 2024

preames Oct 4, 2024

BeMg Oct 7, 2024

topperc Oct 7, 2024


		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.

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

Are you sure you want to change the base?

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

Conversation

BeMg commented Aug 7, 2024

BeMg commented Aug 7, 2024

BeMg commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

topperc commented Aug 22, 2024

BeMg commented Aug 23, 2024

preames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

topperc Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

topperc Sep 25, 2024 •

edited

Loading