On Mon, Jan 15, 2024 at 01:28:04PM +0100, Richard Biener wrote:
> On Mon, Jan 15, 2024 at 12:27 PM Andrew Carlotti
> <[email protected]> wrote:
> >
> > This allows code to determine why a particular function is
> > multiversioned. For now, this will primarily be used to preserve
> > existing name mangling quirks when subsequent commits change all
> > function multiversioning name mangling to use explicit target hooks.
> > However, this can also be used in future to allow more of the
> > multiversioning logic to be moved out of target hooks, and to allow
> > targets to simultaneously enable multiversioning with both 'target' and
> > 'target_version' attributes.
>
> Why does module.cc need to stream the bits? target_clone runs long
> after the FE finished. Instead I wonder why LTO doesn't stream the bits
> (tree-streamer-{in,out}.cc)?
>
> You have four states but only mention 'target' and 'target_version', what's
> the
> states actually? Can you amend the function_version_source enum
> comment accordingly?
All four states are used, although currently not all within a single target,
and in many places you could also work out whether you're in the
"target_clones" case or the "target"/"target_version" case based upon whether
you're in the frontend code or the target_clone pass. So perhaps the second
bit can be made redundant in most places, but it's useful in some parts of the
code.
The main benefits of this design are:
- that it allows backend target hooks to distinguish between the different
causes of function multiversioning without having to create separate hooks,
or pass an extra argument down the call stack;
- that it allows a backend to choose to support mutltiversioning with both
"target" and "target_version" attributes (I remember Martin Liška suggested
supporting "target_version" on x86 as well, and it could provide a way to
improve how multiversioning works without running into backwards
compatibility issues for the "target" attribute).
> This looks like stage1 material to me.
I considered it as stage 3 material, because it fixes a bug on aarch64 (where
the existing code didn't comply with the ACLE spec, and would have cause
problems when applied to public symbols). Admittedly, I did miss the end of
stage 3 by a couple of days; I hadn't realised how early stage 4 began, and
spent the last couple of weeks focussing on Binutils work instead.
However, I've now realised that I can fix this bug entirely within the aarch64
backend (by overwriting the incorrect mangling applied in target-agnostic
code). I'll send out that patch later today, which should hopefully be more
acceptable at this stage.
Thanks,
Andrew
> Thanks,
> Richard.
>
> > gcc/ChangeLog:
> >
> > * multiple_target.cc (expand_target_clones): Use new enum value.
> > * tree-core.h (enum function_version_source): New enum.
> > (struct tree_function_decl): Extend versioned_function to two
> > bits.
> >
> > gcc/cp/ChangeLog:
> >
> > * decl.cc (maybe_mark_function_versioned): Use new enum value.
> > (duplicate_decls): Preserve DECL_FUNCTION_VERSIONED enum value.
> > * module.cc (trees_out::core_bools): Use two bits for
> > function_decl.versioned_function.
> > (trees_in::core_bools): Ditto.
> >
> >
> > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> > index
> > b10a72a87bf0a1cabab52c1e4b657bc8a379b91e..527931cd90a0a779a508a096b2623351fd65a2e8
> > 100644
> > --- a/gcc/cp/decl.cc
> > +++ b/gcc/cp/decl.cc
> > @@ -1254,7 +1254,10 @@ maybe_mark_function_versioned (tree decl)
> > {
> > if (!DECL_FUNCTION_VERSIONED (decl))
> > {
> > - DECL_FUNCTION_VERSIONED (decl) = 1;
> > + if (TARGET_HAS_FMV_TARGET_ATTRIBUTE)
> > + DECL_FUNCTION_VERSIONED (decl) = FUNCTION_VERSION_TARGET;
> > + else
> > + DECL_FUNCTION_VERSIONED (decl) = FUNCTION_VERSION_TARGET_VERSION;
> > /* If DECL_ASSEMBLER_NAME has already been set, re-mangle
> > to include the version marker. */
> > if (DECL_ASSEMBLER_NAME_SET_P (decl))
> > @@ -3159,7 +3162,7 @@ duplicate_decls (tree newdecl, tree olddecl, bool
> > hiding, bool was_hidden)
> > && DECL_FUNCTION_VERSIONED (olddecl))
> > {
> > /* Set the flag for newdecl so that it gets copied to olddecl. */
> > - DECL_FUNCTION_VERSIONED (newdecl) = 1;
> > + DECL_FUNCTION_VERSIONED (newdecl) = DECL_FUNCTION_VERSIONED
> > (olddecl);
> > /* newdecl will be purged after copying to olddecl and is no longer
> > a version. */
> > cgraph_node::delete_function_version_by_decl (newdecl);
> > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > index
> > aa75e2809d8fdca14443c6b911bf725f6d286d20..ba60d0753f91ef91d45fb5d62f26118be4e34840
> > 100644
> > --- a/gcc/cp/module.cc
> > +++ b/gcc/cp/module.cc
> > @@ -5473,7 +5473,11 @@ trees_out::core_bools (tree t)
> > WB (t->function_decl.looping_const_or_pure_flag);
> >
> > WB (t->function_decl.has_debug_args_flag);
> > - WB (t->function_decl.versioned_function);
> > +
> > + /* versioned_function is a 2 bit enum. */
> > + unsigned vf = t->function_decl.versioned_function;
> > + WB ((vf >> 0) & 1);
> > + WB ((vf >> 1) & 1);
> >
> > /* decl_type is a (misnamed) 2 bit discriminator. */
> > unsigned kind = t->function_decl.decl_type;
> > @@ -5618,7 +5622,12 @@ trees_in::core_bools (tree t)
> > RB (t->function_decl.looping_const_or_pure_flag);
> >
> > RB (t->function_decl.has_debug_args_flag);
> > - RB (t->function_decl.versioned_function);
> > +
> > + /* versioned_function is a 2 bit enum. */
> > + unsigned vf = 0;
> > + vf |= unsigned (b ()) << 0;
> > + vf |= unsigned (b ()) << 1;
> > + t->function_decl.versioned_function = function_version_source (vf);
> >
> > /* decl_type is a (misnamed) 2 bit discriminator. */
> > unsigned kind = 0;
> > diff --git a/gcc/multiple_target.cc b/gcc/multiple_target.cc
> > index
> > 1fdd279da04a7acc5e8c50f528139f19cadcd5ff..56a1934fe820e91b2fa451dcf6989382c906b98c
> > 100644
> > --- a/gcc/multiple_target.cc
> > +++ b/gcc/multiple_target.cc
> > @@ -383,7 +383,7 @@ expand_target_clones (struct cgraph_node *node, bool
> > definition)
> > if (decl1_v == NULL)
> > decl1_v = node->insert_new_function_version ();
> > before = decl1_v;
> > - DECL_FUNCTION_VERSIONED (node->decl) = 1;
> > + DECL_FUNCTION_VERSIONED (node->decl) = FUNCTION_VERSION_TARGET_CLONES;
> >
> > for (i = 0; i < attrnum; i++)
> > {
> > @@ -421,7 +421,8 @@ expand_target_clones (struct cgraph_node *node, bool
> > definition)
> >
> > before->next = after;
> > after->prev = before;
> > - DECL_FUNCTION_VERSIONED (new_node->decl) = 1;
> > + DECL_FUNCTION_VERSIONED (new_node->decl)
> > + = FUNCTION_VERSION_TARGET_CLONES;
> > }
> >
> > XDELETEVEC (attrs);
> > diff --git a/gcc/tree-core.h b/gcc/tree-core.h
> > index
> > 8a89462bd7ecac52fcdc11c0b57ccf7c190572b3..e159d53f9d11ba848c49499aa963daa2fbcbc648
> > 100644
> > --- a/gcc/tree-core.h
> > +++ b/gcc/tree-core.h
> > @@ -1955,6 +1955,19 @@ enum function_decl_type
> > /* 0 values left */
> > };
> >
> > +/* Enumerate function multiversioning attributes. This is used to record
> > which
> > + attribute enabled multiversioning on a function, and allows targets to
> > + adjust their behaviour accordingly. */
> > +
> > +enum function_version_source
> > +{
> > + FUNCTION_VERSION_NONE = 0,
> > + FUNCTION_VERSION_TARGET = 1,
> > + FUNCTION_VERSION_TARGET_CLONES = 2,
> > + FUNCTION_VERSION_TARGET_VERSION = 3
> > +};
> > +
> > +
> > /* FUNCTION_DECL inherits from DECL_NON_COMMON because of the use of the
> > arguments/result/saved_tree fields by front ends. It was either
> > inherit
> > FUNCTION_DECL from non_common, or inherit non_common from FUNCTION_DECL,
> > @@ -2002,10 +2015,10 @@ struct GTY(()) tree_function_decl {
> > /* Align the bitfield to boundary of a byte. */
> > ENUM_BITFIELD(function_decl_type) decl_type: 2;
> > unsigned has_debug_args_flag : 1;
> > - unsigned versioned_function : 1;
> > + ENUM_BITFIELD(function_version_source) versioned_function : 2;
> > unsigned replaceable_operator : 1;
> >
> > - /* 11 bits left for future expansion. */
> > + /* 10 bits left for future expansion. */
> > /* 32 bits on 64-bit HW. */
> > };
> >
