On Mon, Feb 10, 2025 at 11:01 PM Martin Jambor <mjam...@suse.cz> wrote:
>
> Hi,
>
> PR 118125 is a performance regression stemming from the fact that we
> lose the cold attribute of our __builtin_unreachable. The attribute
> is simply and silently dropped on the floor by decl_attributes (in
> attribs.cc) in the process of building decls for builtins because it
> cannot look it up in the gnu attribute name space by
> lookup_scoped_attribute_spec. For that not to happen it must be in
> lto_gnu_attributes and this patch adds it there.
>
> In comment 13 of the bug Andrew identified other attributes which are
> in builtin-attrs.def but missing in lto_gnu_attributes but apart from
> cold it seems that they are either not used in builtins.def or are
> used in DEF_LIB_BUILTIN which I guess might be less critical?
> Eventually I decided to go for the most simple of patches and only add
> things if they are requested. For the same reason I also did not add
> any checking to the attribute "handle" callback or any exclusion check.
> They seem to be mostly relevant before LTO FE kicks in to me, but
> again, I'm happy to add any if they seem to be useful.
>
> Since Ian fixed PR 118746, the same issue has also been fixed in the
> Go front-end and so I have added a simple checking assert to the
> redirect_to_unreachable function to make sure it has the intended
> effect.
>
> LTO-bootstrapped and tested on x86_64-linux. OK for master?
OK.
Thanks,
Richard.
> Thanks,
>
> Martin
>
>
> gcc/ChangeLog:
>
> 2025-02-03 Martin Jambor <mjam...@suse.cz>
>
> PR lto/118125
> * ipa-fnsummary.cc (redirect_to_unreachable): Add checking assert
> that the builtin_unreachable decl has attribute cold.
>
> gcc/lto/ChangeLog:
>
> 2025-02-03 Martin Jambor <mjam...@suse.cz>
>
> PR lto/118125
> * lto-lang.cc (lto_gnu_attributes): Add an entry for cold attribute.
> (handle_cold_attribute): New function.
> ---
> gcc/ipa-fnsummary.cc | 3 +++
> gcc/lto/lto-lang.cc | 13 +++++++++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/gcc/ipa-fnsummary.cc b/gcc/ipa-fnsummary.cc
> index 33f19365ec3..4c062fe8a0e 100644
> --- a/gcc/ipa-fnsummary.cc
> +++ b/gcc/ipa-fnsummary.cc
> @@ -255,6 +255,9 @@ redirect_to_unreachable (struct cgraph_edge *e)
> struct cgraph_node *target
> = cgraph_node::get_create (builtin_decl_unreachable ());
>
> + gcc_checking_assert (lookup_attribute ("cold",
> + DECL_ATTRIBUTES (target->decl)));
> +
> if (e->speculative)
> e = cgraph_edge::resolve_speculation (e, target->decl);
> else if (!e->callee)
> diff --git a/gcc/lto/lto-lang.cc b/gcc/lto/lto-lang.cc
> index 652d7fc5e30..e41b548b398 100644
> --- a/gcc/lto/lto-lang.cc
> +++ b/gcc/lto/lto-lang.cc
> @@ -60,6 +60,7 @@ static tree ignore_attribute (tree *, tree, tree, int, bool
> *);
> static tree handle_format_attribute (tree *, tree, tree, int, bool *);
> static tree handle_fnspec_attribute (tree *, tree, tree, int, bool *);
> static tree handle_format_arg_attribute (tree *, tree, tree, int, bool *);
> +static tree handle_cold_attribute (tree *, tree, tree, int, bool *);
>
> /* Helper to define attribute exclusions. */
> #define ATTR_EXCL(name, function, type, variable) \
> @@ -128,6 +129,8 @@ static const attribute_spec lto_gnu_attributes[] =
> handle_sentinel_attribute, NULL },
> { "type generic", 0, 0, false, true, true, false,
> handle_type_generic_attribute, NULL },
> + { "cold", 0, 0, false, false, false, false,
> + handle_cold_attribute, NULL },
> { "fn spec", 1, 1, false, true, true, false,
> handle_fnspec_attribute, NULL },
> { "transaction_pure", 0, 0, false, true, true, false,
> @@ -598,6 +601,16 @@ handle_fnspec_attribute (tree *node ATTRIBUTE_UNUSED,
> tree ARG_UNUSED (name),
> return NULL_TREE;
> }
>
> +/* Handle a "cold" attribute; arguments as in
> + struct attribute_spec.handler. */
> +
> +static tree
> +handle_cold_attribute (tree *, tree, tree, int, bool *)
> +{
> + /* Nothing to be done here. */
> + return NULL_TREE;
> +}
> +
> /* Cribbed from c-common.cc. */
>
> static void
> --
> 2.47.1
>
