Hi Alfie,
On 10/8/25 14:57, [email protected] wrote:
From: Alfie Richards <[email protected]>
Adds support for the AArch64 fmv priority syntax.
This allows users to override the default function ordering.
For example:
```c
int bar [[gnu::target_version("default")]] (int){
return 1;
}
int bar [[gnu::target_version("dotprod;priority=2")]] (int) {
return 2;
}
int bar [[gnu::target_version("sve;priority=1")]] (int) {
return 3;
}
```
gcc/ChangeLog:
* config/aarch64/aarch64.cc (aarch64_parse_fmv_features): Add parsing
for priority arguments.
(aarch64_process_target_version_attr): Update call to
aarch64_parse_fmv_features.
(get_feature_mask_for_version): Update call to
aarch64_parse_fmv_features.
(aarch64_compare_version_priority): Add logic to order by priority if
present.
(aarch64_functions_b_resolvable_from_a): Update call to
aarch64_parse_fmv_features.
(aarch64_mangle_decl_assembler_name): Update call to
aarch64_parse_fmv_features.
(dispatch_function_versions): Add logic to sort by priority.
(aarch64_same_function_versions): Add diagnostic if invalid use of
priority syntax.
(aarch64_merge_decl_attributes): Add logic to make suer priority
arguments are preserved.
(aarch64_check_target_clone_version): Update call to
aarch64_parse_fmv_features.
gcc/testsuite/ChangeLog:
* gcc.target/aarch64/fmv_priority3.c: New test.
---
gcc/config/aarch64/aarch64.cc | 126 +++++++++++++++---
.../gcc.target/aarch64/fmv_priority3.c | 24 ++++
2 files changed, 130 insertions(+), 20 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/aarch64/fmv_priority3.c
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 3f341f33f0a..6cfd269c07b 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -20403,21 +20403,27 @@ static aarch64_fmv_feature_datum
aarch64_fmv_feature_data[] = {
static enum aarch_parse_opt_result
aarch64_parse_fmv_features (string_slice str, aarch64_feature_flags
*isa_flags,
aarch64_fmv_feature_mask *feature_mask,
+ unsigned int *priority,
std::string *invalid_extension)
{
if (feature_mask)
*feature_mask = 0ULL;
+ if (priority)
+ *priority = 0;
+
+ str = str.strip ();
if (str == "default")
return AARCH_PARSE_OK;
- gcc_assert (str.is_valid ());
-
- while (str.is_valid ())
+ /* Parse the architecture part of the string. */
+ string_slice arch_string = string_slice::tokenize (&str, ";");
+ gcc_assert (arch_string.is_valid ());
Redundant assert - it's always valid even if empty, and we're testing it
immediately below...
+ while (arch_string.is_valid ())
{
string_slice ext;
- ext = string_slice::tokenize (&str, "+");
+ ext = string_slice::tokenize (&arch_string, "+");
gcc_assert (ext.is_valid ());
@@ -20458,6 +20464,60 @@ aarch64_parse_fmv_features (string_slice str, aarch64_feature_flags *isa_flags,
}
}
+ /* Parse any extra arguments. */
+ while (str.is_valid ())
+ {
+ string_slice argument = string_slice::tokenize (&str, ";");
+ string_slice arg = argument;
+ char n[4];
+ argument = argument.strip ();
Wouldn't it make sense to strip spaces before arg = argument?
+ string_slice name = string_slice::tokenize (&argument, "=").strip ();
+ unsigned int priority_res = 0;
+
+ /* priority=N argument (only one is allowed). */
+ if (name == "priority" && priority_res == 0)
+ {
+ if (!argument.is_valid ())
+ {
+ *invalid_extension = std::string (arg.begin (), arg.size ());
+ return AARCH_PARSE_INVALID_FEATURE;
+ }
+ argument = argument.strip ();
+ if (!argument.is_valid ())
+ {
+ *invalid_extension = std::string (arg.begin (), arg.size ());
+ return AARCH_PARSE_INVALID_FEATURE;
+ }
This is dead code, it if was valid, it remains valid after stripping spaces.
+ if (argument.size () > 3)
Since the slice can be empty, we should also check argument.size () > 0.
+ {
+ *invalid_extension = std::string (arg.begin (), arg.size ());
+ return AARCH_PARSE_INVALID_FEATURE;
+ }
+ memcpy (n, argument.begin (),
+ std::min (argument.size (), (unsigned int) 3));
What's the idea behind the ::min? We already know size() <= 3...
+
+ int res = sscanf (n, "%ud", &priority_res);
This will return random results and can even read beyond the size of the
array. You need to terminate the string. Also "%ud" will math "+1" which
you probably don't want to match.
+ if (res != 1)
+ {
+ *invalid_extension = std::string (arg.begin (), arg.size ());
+ return AARCH_PARSE_INVALID_FEATURE;
+ }
+ if (priority_res < 1 || priority_res > 255)
+ {
+ *invalid_extension = std::string (arg.begin (), arg.size ());
+ return AARCH_PARSE_INVALID_FEATURE;
+ }
I'd merge this if into the previous if and check all 3 conditions in one go.
+ if (priority)
+ *priority = priority_res;
+ }
+ else
+ {
+ /* Unrecognised argument. */
+ *invalid_extension = std::string (arg.begin (), arg.size ());
+ return AARCH_PARSE_INVALID_FEATURE;
+ }
+ }
+
return AARCH_PARSE_OK;
}
@@ -20489,7 +20549,7 @@ aarch64_process_target_version_attr (tree args)
auto isa_flags = aarch64_asm_isa_flags;
std::string invalid_extension;
- parse_res = aarch64_parse_fmv_features (str, &isa_flags, NULL,
+ parse_res = aarch64_parse_fmv_features (str, &isa_flags, NULL, NULL,
&invalid_extension);
if (parse_res == AARCH_PARSE_OK)
@@ -20578,7 +20638,7 @@ aarch64_option_valid_version_attribute_p (tree fndecl,
tree, tree args, int)
add or remove redundant feature requirements. */
static aarch64_fmv_feature_mask
-get_feature_mask_for_version (tree decl)
+get_feature_mask_for_version (tree decl, unsigned int *priority)
{
tree version_attr = lookup_attribute ("target_version",
DECL_ATTRIBUTES (decl));
@@ -20592,7 +20652,7 @@ get_feature_mask_for_version (tree decl)
aarch64_fmv_feature_mask feature_mask;
parse_res = aarch64_parse_fmv_features (version_string, NULL,
- &feature_mask, NULL);
+ &feature_mask, priority, NULL);
/* We should have detected any errors before getting here. */
gcc_assert (parse_res == AARCH_PARSE_OK);
@@ -20628,9 +20688,13 @@ compare_feature_masks (aarch64_fmv_feature_mask mask1,
int
aarch64_compare_version_priority (tree decl1, tree decl2)
{
- auto mask1 = get_feature_mask_for_version (decl1);
- auto mask2 = get_feature_mask_for_version (decl2);
+ unsigned int priority_1 = 0;
+ unsigned int priority_2 = 0;
+ auto mask1 = get_feature_mask_for_version (decl1, &priority_1);
+ auto mask2 = get_feature_mask_for_version (decl2, &priority_2);
+ if (priority_1 != priority_2)
+ return priority_1 > priority_2 ? 1 : -1;
return compare_feature_masks (mask1, mask2);
}
@@ -20647,9 +20711,9 @@ aarch64_functions_b_resolvable_from_a (tree decl_a, tree decl_b, tree baseline)
auto a_version = get_target_version (decl_a);
auto b_version = get_target_version (decl_b);
if (a_version.is_valid ())
- aarch64_parse_fmv_features (a_version, &isa_a, NULL, NULL);
+ aarch64_parse_fmv_features (a_version, &isa_a, NULL, NULL, NULL);
if (b_version.is_valid ())
- aarch64_parse_fmv_features (b_version, &isa_b, NULL, NULL);
+ aarch64_parse_fmv_features (b_version, &isa_b, NULL, NULL, NULL);
/* Are there any bits of b that arent in a. */
if (isa_b & (~isa_a))
@@ -20727,7 +20791,7 @@ aarch64_mangle_decl_assembler_name (tree decl, tree id)
else if (DECL_FUNCTION_VERSIONED (decl))
{
aarch64_fmv_feature_mask feature_mask
- = get_feature_mask_for_version (decl);
+ = get_feature_mask_for_version (decl, NULL);
if (feature_mask == 0ULL)
return clone_identifier (id, "default");
@@ -21157,19 +21221,27 @@ aarch64_get_function_versions_dispatcher (void *decl)
function. */
bool
-aarch64_same_function_versions (string_slice str1, string_slice str2)
+aarch64_same_function_versions (string_slice old_str, string_slice new_str)
{
enum aarch_parse_opt_result parse_res;
aarch64_fmv_feature_mask feature_mask1;
aarch64_fmv_feature_mask feature_mask2;
- parse_res = aarch64_parse_fmv_features (str1, NULL,
- &feature_mask1, NULL);
+ unsigned int priority1;
+ unsigned int priority2;
+ parse_res = aarch64_parse_fmv_features (old_str, NULL,
+ &feature_mask1, &priority1, NULL);
gcc_assert (parse_res == AARCH_PARSE_OK);
- parse_res = aarch64_parse_fmv_features (str2, NULL,
- &feature_mask2, NULL);
+ parse_res = aarch64_parse_fmv_features (new_str, NULL,
+ &feature_mask2, &priority2, NULL);
gcc_assert (parse_res == AARCH_PARSE_OK);
- return feature_mask1 == feature_mask2;
+ if (feature_mask1 != feature_mask2)
+ return false;
+
+ if (priority1 != priority2 && priority1 != 0)
+ error ("inconsistent priority values");
Is it correct that this is asymmetric like that?
+
+ return true;
}
/* Implement TARGET_FUNCTION_ATTRIBUTE_INLINABLE_P. Use an opt-out
@@ -30276,6 +30348,20 @@ aarch64_merge_decl_attributes (tree olddecl, tree
newdecl)
aarch64_check_arm_new_against_type (TREE_VALUE (new_new), newdecl);
}
+ /* For target_version and target_clones, make sure we take the old version
+ as priority syntax cannot be added addetively. */
+ tree old_target_version = lookup_attribute ("target_version", old_attrs);
+ tree new_target_version = lookup_attribute ("target_version", new_attrs);
+
+ if (old_target_version && new_target_version)
+ TREE_VALUE (new_target_version) = TREE_VALUE (old_target_version);
+
+ tree old_target_clones = lookup_attribute ("target_clones", old_attrs);
+ tree new_target_clones = lookup_attribute ("target_clones", new_attrs);
+
+ if (old_target_clones && new_target_clones)
+ TREE_VALUE (new_target_clones) = TREE_VALUE (old_target_clones);
+
return merge_attributes (old_attrs, new_attrs);
}
@@ -31944,8 +32030,8 @@ aarch64_check_target_clone_version (string_slice str, location_t *loc)
auto isa_flags = aarch64_asm_isa_flags;
std::string invalid_extension;
- parse_res
- = aarch64_parse_fmv_features (str, &isa_flags, NULL, &invalid_extension);
+ parse_res = aarch64_parse_fmv_features (str, &isa_flags, NULL, NULL,
+ &invalid_extension);
if (loc == NULL)
return parse_res == AARCH_PARSE_OK;
diff --git a/gcc/testsuite/gcc.target/aarch64/fmv_priority3.c
b/gcc/testsuite/gcc.target/aarch64/fmv_priority3.c
new file mode 100644
index 00000000000..0f24bf7c798
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/fmv_priority3.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-require-ifunc "" } */
+/* { dg-options "-O0 -fdump-ipa-targetclone1-details" } */
+
+int foo [[gnu::target_version("default")]] (int) { return 1; }
+int foo [[gnu::target_version("dotprod;priority=1")]] (int) { return 2; }
+int foo [[gnu::target_version("sve")]] (int) { return 3; }
+
+// Priority1 dotprod > sve
+/* { dg-final { scan-ipa-dump-times "Version order for
foo/\[0-9\]+:\\nfoo\.default/\[0-9\]+\\nfoo\._Msve/\[0-9\]+\\nfoo\._Mdotprod/\[0-9\]+\\n" 1
"targetclone1" } } */
+
+int bar [[gnu::target_version("default")]] (int) { return 1; }
+int bar [[gnu::target_version("dotprod;priority=2")]] (int) { return 2; }
+int bar [[gnu::target_version("sve;priority=1")]] (int) { return 3; }
+
+// Priority2 dotprod > Priority1 sve
+/* { dg-final { scan-ipa-dump-times "Version order for
bar/\[0-9\]+:\\nbar\.default/\[0-9\]+\\nbar\._Msve/\[0-9\]+\\nbar\._Mdotprod/\[0-9\]+\\n" 1
"targetclone1" } } */
+
+int baz [[gnu::target_version("default")]] (int) { return 1; }
+int baz [[gnu::target_version("dotprod;priority=1")]] (int) { return 2; }
+int baz [[gnu::target_version("sve;priority=1")]] (int) { return 3; }
+
+// Priority1 sve > Priority1 dotprod
+/* { dg-final { scan-ipa-dump-times "Version order for
baz/\[0-9\]+:\\nbaz\.default/\[0-9\]+\\nbaz\._Mdotprod/\[0-9\]+\\nbaz\._Msve/\[0-9\]+\\n" 1
"targetclone1" } } */
