he 10/09/2025 16:27, Wilco Dijkstra wrote:
> Hi Alfie,
Hi Wilco,
Thanks for the review!
(email slightly hacked apart for brevity)
> > 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...
Yup agreed.
>
> > + 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?
Yup thanks, my bad.
>
> > + 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.
Ugh, yes this is supposed to be empty(), will fix.
>
> > + 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.
Do you know what function I can be use instead just to parse an unsigned int?
>
> > + 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.
Will do, thanks!
>
> > + 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;
> > }
> > -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?
Yup.
We allow
int bar [[gnu::target_version("dotprod;priority=2")]] (int);
int bar [[gnu::target_version("dotprod")]] (int) { return 1; }
But not
int bar [[gnu::target_version("dotprod")]] (int);
int bar [[gnu::target_version("dotprod;priority=2")]] (int) { return 1; }
As the first example can merge the priority downwards simply. But in the other
case
working out the priority at different points is confusing and unsound.
>
> > +
> > + 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);
> > +
>
--
Alfie Richards
