Added missing CC
On 05/02/2025 17:14, Richard Sandiford wrote:
Alfie Richards <alfie.richa...@arm.com> writes:
On 04/02/2025 10:03, Richard Sandiford wrote:
Alfie Richards <alfie.richa...@arm.com> writes:
+ return id;
+ else if (cgraph_node::get_create
(decl)->dispatcher_resolver_function)
+ id = clone_identifier (id, "resolver");
+ else if (DECL_FUNCTION_VERSIONED (decl))
{
- name += ".default";
- return get_identifier (name.c_str());
- }
-
- name += "._";
+ aarch64_fmv_feature_mask feature_mask
+ = get_feature_mask_for_version (decl);
- int num_features = ARRAY_SIZE (aarch64_fmv_feature_data);
- for (int i = 0; i < num_features; i++)
- {
- if (feature_mask & aarch64_fmv_feature_data[i].feature_mask)
+ if (feature_mask == 0ULL) // && not implemented!
Could you explain the // comment?
Apologies, this was a temporary marker for myself that slipped through.
{
- name += "M";
- name += aarch64_fmv_feature_data[i].name;
+ if (!DECL_INITIAL (decl))
+ return id;
+ return clone_identifier (id, "default");
This different handling of defined vs. undefined functions feels a
bit weird. It's not clear to me whether:
(1) The .default version is effectively internal to the translation
unit,
and therefore other translation units cannot assume it exists.
(2) Other translation units can assume that the .default version exists
if they can see a target_version("default") or target_clones
definition.
(3) Other translation units can assume that the .default version exists
if they can see a non-default target_version or a
target_clones definition.
(4) Something else.
Argh, sorry, I meant declaration rather than definition for both
(2) and (3). My question doesn't make sense as written.
(2) would create a difference between implicit and explicit defaults
and doesn't seem to be what the series implements (mv-symbols6.C from
patch 14). (3) seems indirect, and IMO it's dangerous to assume that
the mere existence of a non-default version X in one TU implies that
version X will be visible in the TU that contains the resolver. I
would
have expected that it would be valid for version X not to be visible
in the TU that contains the resolver, with the consequence that the
resolver won't use it.
(1) seems more appealing on the face of it, so that .default is more
like .resolver. But that doesn't seem to be what the spec says.
I would say its (4) Something else.
This code is the result of that and a discussion we had where we
decided we
can avoid mangling the default version symbol if it is external. As we
know that
in the TU where the default is defined, the default
version will be mangled, and the dispatched symbol will take its name.
As the default is the only resolvable version, then all calls and
references
will already have the correct target.
This allows us to avoid making the dispatched symbol and redirecting
default decl calls/references to the dispatched symbol.
That sounds like what I meant by (1): i.e. that the only TU that can
reference .default is the one that creates it (i.e. the one with the
definition). A declaration with
__attribute__((target_version("default"))
doesn't itself imply that a .default version exists.
The reason for asking is that the ACLE says:
The "default" version is mangled with ".default" on top of the
language-specific name mangling. All versioned functions with their
mangled names are always resolvable.
which made it sound like other TUs could rely on ".default" existing.
Ahh that does help. I follow now.
My current understanding comes from following the clang implementation
and discussions regarding the spec.
But, I see your point though that this isn't what is specified.
I think a good correct solution would be that if there is
target_version ("default") then to always mangle the default. Then in the
case that there's a default and no other versions, emit a
non-mangled symbol alias to the default version?
Then we cover all cases well, follow the spec, and will be broadly
compatible
with clang. (ie. in that case currently clang emits the default as if it
wasn't annotated, we would emit the default mangled, and include an alias
to the unmangled symbol)
It's a bit of a hack, and we can instead always mangle the default,
create the dispatched symbol? Apologies if I misunderstood the earlier
conversation.
}
- }
- if (DECL_ASSEMBLER_NAME_SET_P (decl))
- SET_DECL_RTL (decl, NULL);
+ std::string suffix = "_";
+
+ int num_features = ARRAY_SIZE (aarch64_fmv_feature_data);
+ for (int i = 0; i < num_features; i++)
+ if (feature_mask & aarch64_fmv_feature_data[i].feature_mask)
+ {
+ suffix += "M";
+ suffix += aarch64_fmv_feature_data[i].name;
+ }
+
+ if (DECL_ASSEMBLER_NAME_SET_P (decl))
+ SET_DECL_RTL (decl, NULL);
Why is it necessary to conditionally reset the DECL_RTL for the
non-default case but not necessary when adding ".default"?
Thanks,
Richard
To be honest I don’t understand this code particularly well. This was
here
previously, and I understood it to imply that the function needs to be
recompiled if it is non-default as it's architecture features will
have changed.I think it only really applies in the target_clones case
where the new
decl from expand_clones could have carried over the RTL of the default?
Resetting DECL_RTL doesn't AFAIK trigger any recompilation.
IIRC, the DECL_RTL for a function is usually:
(mem (symbol_ref "<mangled name>"))
and so the purpose of clearing it out is to force a new symbol_ref
(with a new mangling) to be created.
It would be interesting to see whether any failures are visible with the
SET_DECL_RTL removed. If so, it would be interesting to know whether
there's a particular reason why the same failure couldn't trigger for
the default version. If not, then maybe the code can be removed.
(I'd expect a failure, but I'm certainly not 100% sure.)
Ah that's super helpful to know, thank you!
I will experimentfor what's needed.
Thanks,
Richard
