Hi Alfie,
> This changes the hook from being symmetric, to explicitly taking a version
> string from the old decl and one from the new decl.
>
> Additionally, it documents that we support emitting a diagnostic if the
> strings
> do imply the same version, but are incompatible.
It would be good if there was clearer that new_decl is always a later
declaration.
Note the swapping makes the naming kind of wrong - see comments.
> This target hook returns @code{true} if the target/target-version strings
> -@var{fn1} and @var{fn2} imply the same function version.
> +@var{old_str} and @var{new_str} imply the same function version.
> +Can also produce a diagnostic if the version strings do imply the same
> +version, but are incompatible.
Perhaps add something here that makes it clear that new_str is from a later
declaration or definition.
> -/* This function returns true if FN1 and FN2 define the same version of a
> - function. */
> +/* This function returns true if OLD_STR and NEW_STR define the same version
> + of a function. */
Same here.
/* Checks if we can be certain that function DECL_A could resolve DECL_B. */
diff --git a/gcc/tree.cc b/gcc/tree.cc
index 18abd1f3c4b..8a294a646cc 100644
--- a/gcc/tree.cc
+++ b/gcc/tree.cc
@@ -15499,25 +15499,26 @@ get_target_version (const tree decl)
.strip ();
}
@@ -15528,24 +15529,25 @@ disjoint_version_decls (tree fn1, tree fn2)
multi-versioned. */
if (attr1 == NULL_TREE || attr2 == NULL_TREE)
{
- if (DECL_FUNCTION_VERSIONED (fn1) || DECL_FUNCTION_VERSIONED (fn2))
+ if (DECL_FUNCTION_VERSIONED (new_decl)
+ || DECL_FUNCTION_VERSIONED (old_decl))
{
if (attr2 != NULL_TREE)
{
- std::swap (fn1, fn2);
+ std::swap (new_decl, old_decl);
attr1 = attr2;
Here is where it gets confusing...
}
auto_diagnostic_group d;
- error_at (DECL_SOURCE_LOCATION (fn2),
+ error_at (DECL_SOURCE_LOCATION (old_decl),
"missing %<target%> attribute for multi-versioned %qD",
- fn2);
- inform (DECL_SOURCE_LOCATION (fn1),
- "previous declaration of %qD", fn1);
+ old_decl);
+ inform (DECL_SOURCE_LOCATION (new_decl),
+ "previous declaration of %qD", new_decl);
Is this still a previous declaration???
/* Prevent diagnosing of the same error multiple times. */
- DECL_ATTRIBUTES (fn2)
+ DECL_ATTRIBUTES (old_decl)
= tree_cons (get_identifier ("target"),
copy_node (TREE_VALUE (attr1)),
- DECL_ATTRIBUTES (fn2));
+ DECL_ATTRIBUTES (old_decl));
So do we want to update the previous or latest declaration here?
/* As this is symmetric, can remove the case where fn2 is target clone
and fn1 is target version by swapping here. */
- if (lookup_attribute ("target_clones", DECL_ATTRIBUTES (fn2)))
- std::swap (fn1, fn2);
+ bool swapped = false;
+ if (lookup_attribute ("target_clones", DECL_ATTRIBUTES (old_decl)))
+ {
+ swapped = true;
+ std::swap (new_decl, old_decl);
+ }
Same here, I'm confused...
- if (lookup_attribute ("target_clones", DECL_ATTRIBUTES (fn1)))
+ if (lookup_attribute ("target_clones", DECL_ATTRIBUTES (new_decl)))
{
- auto_vec<string_slice> fn1_versions = get_clone_versions (fn1);
+ auto_vec<string_slice> new_versions = get_clone_versions (new_decl);
/* fn1 is target_clone. */
- if (lookup_attribute ("target_clones", DECL_ATTRIBUTES (fn2)))
+ if (lookup_attribute ("target_clones", DECL_ATTRIBUTES (old_decl)))
{
/* Both are target_clone. */
- auto_vec<string_slice> fn2_versions = get_clone_versions (fn2);
- for (string_slice v1 : fn1_versions)
+ auto_vec<string_slice> old_versions
+ = get_clone_versions (old_decl);
+ for (string_slice new_v : new_versions)
{
- for (string_slice v2 : fn2_versions)
- if (targetm.target_option.same_function_versions (v1, v2))
- return false;
+ for (string_slice old_v : old_versions)
+ {
+ if (swapped)
+ std::swap (new_v, old_v);
+ if (targetm.target_option.same_function_versions (old_v,
+ new_v))
+ return false;
+ }
Swapping in a doubly nested loop becomes even more confusing...
- string_slice v2 = get_target_version (fn2);
+ string_slice old_v = get_target_version (old_decl);
/* target and target_clones is always conflicting for target
semantics. */
@@ -15595,29 +15607,34 @@ disjoint_version_decls (tree fn1, tree fn2)
return false;
/* Only fn1 is target clone. */
- if (!v2.is_valid ())
- v2 = "default";
- for (string_slice v1 : fn1_versions)
- if (targetm.target_option.same_function_versions (v1, v2))
- return false;
+ if (!old_v.is_valid ())
+ old_v = "default";
+ for (string_slice new_v : new_versions)
+ {
+ if (swapped)
+ std::swap (new_v, old_v);
+ if (targetm.target_option.same_function_versions (old_v,
+ new_v))
+ return false;
+ }
Again here.
/* Both are target_version. */
- string_slice v1 = get_target_version (fn1);
- string_slice v2 = get_target_version (fn2);
+ string_slice new_v = get_target_version (new_decl);
+ string_slice old_v = get_target_version (old_decl);
- if (!v1.is_valid () && !v2.is_valid ())
+ if (!new_v.is_valid () && !old_v.is_valid ())
return false;
- if (!v1.is_valid ())
- v1 = "default";
- if (!v2.is_valid ())
- v2 = "default";
+ if (!new_v.is_valid ())
+ new_v = "default";
+ if (!old_v.is_valid ())
+ old_v = "default";
- if (targetm.target_option.same_function_versions (v1, v2))
+ if (targetm.target_option.same_function_versions (old_v, new_v))
return false;
Should this not do a swap given we've swapped above?
I'd suggest to never swap anything named "old" and "new" since that makes things
very confusing. It's not clear how much code is being saved from duplication,
but it may be feasible to abstract out code that is oblivious to the swap and
structure it like this:
if (old is X != new is X)
{
// special processing based on first arg being X
if (old is X) do_thing (old, new)
else do_thing (new, old)
// continue using old and new
}
Cheers,
Wilco
