[2/5] tree: Extend DECL_FUNCTION_VERSIONED to an enum

  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.

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.
  

On Mon, Jan 15, 2024 at 12:27 PM Andrew Carlotti
<andrew.carlotti@arm.com> 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?

This looks like stage1 material to me.

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.  */
>  };
>
  

On Mon, Jan 15, 2024 at 01:28:04PM +0100, Richard Biener wrote:
> On Mon, Jan 15, 2024 at 12:27 PM Andrew Carlotti
> <andrew.carlotti@arm.com> 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.  */
> >  };
> >