On 11/26/24 9:35 AM, alfie.richa...@arm.com wrote:
This patch adds the TARGET_CREATE_FMV_DISPATCHER_AT_DEFAULT_IMPL flag
which changes FMV behavior for target_version functions to match the Arm
C Language Extension.
The functional differences consist of:
1. Generating the resolver for the dispatched symbol at the site of the
default version definition.
2. Mangling non-default FMV annotated functions even when no other
versions are present.
This allows for better behavior when definitions are spread across
different TU's as one resolver will be created and the
[ACLE](https://github.com/ARM-software/acle/blob/main/main/acle.md#function-multi-versioning)
stipulates that the default implementation must have visibility of all
other versions so that resolver will be complete. This also matches
Clangs behavior.
The only remaining discrepancy I'm aware of when targeting AArch64 is we
do not allow the coexistence of target_version and target_clone, which
is specified as supported in the ACLE but will be addressed later.
Note this hook is only true on Aarch64, so other targets will not be
affected.
To enable these functionality changes I added mangling to the initial
processing of functions in the CPP frontend, and changed the
Please don't refer to C++ as CPP, to me that means the C preprocessor.
logic for the creation of resolver bodies to create at default
declaration implementations.
Additionally, the previous naming logic relied on the fact that if there
existed a call to a dispatched function the resolver would also be
created which would do some renaming. As that no longer is guaranteed
this patch hacks on the assembler names to make them correct.
@@ -1250,13 +1250,17 @@ common_function_versions (tree fn1, tree fn2)
by the front-end. Return the decl created. */
tree
-make_dispatcher_decl (const tree decl)
+make_dispatcher_decl (const tree decl, const char *name)
The function comment needs updating for the new parameter.
+/* If allow_unversioned is false, returns true if DECL has been marked as
+ multiversioned, is multi-versioned using the an attribute, and this is
"the an"?
We conventionally refer to parameter names in all caps (i.e.
ALLOW_UNVERSIONED) in the function comment.
+ the default version.
+ If allow_unversioned is true, then does not require the DECL is marked as
+ versioned and returns true if the function could be a default function,
+ ie could be unannotated. */
I think it would actually be clearer to leave the current first sentence
alone other than changing "the target attribute" to "an attribute", and
say that the function additionally returns true in this case if
allow_unversioned.
Actually, why does this need to be a parameter at all? Why not
condition it on the target macro?
bool
-is_function_default_version (const tree decl)
+is_function_default_version (const tree decl, bool allow_unversioned)
+ = lookup_attribute (TARGET_HAS_FMV_TARGET_ATTRIBUTE ? "target"
+ : "target_version",
That indentation won't last, let's move the ? to the second line as well.
-extern tree make_dispatcher_decl (const tree);
-extern bool is_function_default_version (const tree);
+extern tree
+make_dispatcher_decl (const tree, const char *name = NULL);
+extern bool
+is_function_default_version (const tree, bool = false);
We only put the function name at the beginning of the line for
definitions, not forward declarations.
+ /* Check for the default version. */
+ if (TARGET_CREATE_FMV_DISPATCHER_AT_DEFAULT_IMPL
+ && !is_function_default_version (
+ function_version ()->next->this_node->decl, true)
+ && callers)
We don't put ( at the end of a line.
Why are we calling function_version () again here instead of referring
to version_info?
+ /* Generate the dispatcher body of multi-versioned functions at
+ the first point where the dispatched symbol has been called. */
+ if ((!TARGET_CREATE_FMV_DISPATCHER_AT_DEFAULT_IMPL
+ || (TARGET_CREATE_FMV_DISPATCHER_AT_DEFAULT_IMPL && version_info
+ && version_info->next
+ && is_function_default_version (
+ version_info->next->this_node->decl, true)
This is the exact same condition as above, let's use a local variable to
store its value.
+ /* We need to always process the dispatcher resolver in the
+ TARGET_CREATE_FMV_DISPATCHER_AT_DEFAULT_IMPL fmv case. */
+ if (version_info && TARGET_CREATE_FMV_DISPATCHER_AT_DEFAULT_IMPL
+ && !dispatcher_function && is_function_default_version (decl, true))
+ enqueue_node (cgraph_node::get_create (
+ targetm.get_function_versions_dispatcher (decl)));
Again no ( at end of line.
@@ -20364,6 +20364,18 @@ aarch64_mangle_decl_assembler_name (tree decl, tree id)
return id;
}
+static std::string
+get_assembler_name_without_default (tree default_decl)
+{
+ std::string name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (default_decl));
+
+ auto size = name.size ();
+ if (size >= 8 && name.compare (size - 8, 8, ".default") == 0)
+ name.resize (size - 8);
+
+ return name;
+}
This function needs a comment.
@@ -20839,8 +20847,19 @@ aarch64_get_function_versions_dispatcher (void *decl)
+ /* Mark the assembler name as set to prevent it getting mangled again .*/
+ SET_DECL_ASSEMBLER_NAME (dispatch_decl,
+ DECL_ASSEMBLER_NAME (dispatch_decl));
This doesn't make sense to me. When you refer to DECL_ASSEMBLER_NAME it
gets set; passing the result to SET_DECL_ASSEMBLER_NAME shouldn't have
any effect. I think you can lose this whole statement.
@@ -18409,6 +18406,15 @@ start_preparsed_function (tree decl1, tree attrs, int
flags)
if (!DECL_OMP_DECLARE_REDUCTION_P (decl1))
start_lambda_scope (decl1);
+ /* To enable versions to be created across TU's we mark and mangle all
+ non-default versioned functions. */
+ if (TARGET_CREATE_FMV_DISPATCHER_AT_DEFAULT_IMPL
+ && lookup_attribute (TARGET_HAS_FMV_TARGET_ATTRIBUTE ? "target"
+ : "target_version",
+ DECL_ATTRIBUTES (decl1)))
+ if (!is_function_default_version (decl1, true))
+ maybe_mark_function_versioned (decl1);
The lookup_attribute seems redundant with !is_function_default_version.
+ extension and esures batter behavior when defining function versions
"ensures better"
Jason
