[RFC] c++: extend cold, hot attributes to classes

  Most code is cold. This patch extends support for attribute ((cold)) to C++
Classes, Unions, and Structs (RECORD_TYPES and UNION_TYPES) to benefit from
encapsulation - reducing the verbosity of using the attribute where
deserved. The ((hot)) attribute is also extended for its semantic relation.
What is the sentiment on this use-case?

for gcc/c-family/ChangeLog

    * c-attribs.c (attribute_spec): Remove decl_required
    field for "hot" and "cold" attributes.
    (handle_cold_attribute): remove warning on RECORD_TYPE
    and UNION_TYPE when c_dialect_cxx.
    (handle_cold_attribute): remove warning on RECORD_TYPE
    and UNION_TYPE when c_dialect_cxx.

for gcc/cp/ChangeLog

    * class.c (finish_struct) propagate hot and cold
    attributes to all FUNCTION_DECL when the class
    itself is marked hot or cold.

for  gcc/testsuite/ChangeLog

    * g++.dg/ext/attr-hotness.C: New.


Signed-off-by: Javier Martinez <javier.martinez.bugzilla@gmail.com>

foo(void) { } }; /* { dg-warning "ignoring attribute .hot. because it
conflicts with attribute .cold." } */
+
+/* { dg-final { scan-tree-dump-times "cold" 2 "gimple" } } */
+/* { dg-final { scan-tree-dump-times "hot" 2 "gimple" } } */
+
  

On 8/3/23 07:07, Javier Martinez via Gcc-patches wrote:
> Most code is cold. This patch extends support for attribute ((cold)) to C++
> Classes, Unions, and Structs (RECORD_TYPES and UNION_TYPES) to benefit from
> encapsulation - reducing the verbosity of using the attribute where
> deserved. The ((hot)) attribute is also extended for its semantic relation.
> What is the sentiment on this use-case?

Seems reasonable, but how do you expect this to be used?

> for gcc/c-family/ChangeLog
> 
>      * c-attribs.c (attribute_spec): Remove decl_required
>      field for "hot" and "cold" attributes.
>      (handle_cold_attribute): remove warning on RECORD_TYPE
>      and UNION_TYPE when c_dialect_cxx.
>      (handle_cold_attribute): remove warning on RECORD_TYPE
>      and UNION_TYPE when c_dialect_cxx.
> 
> for gcc/cp/ChangeLog
> 
>      * class.c (finish_struct) propagate hot and cold
>      attributes to all FUNCTION_DECL when the class
>      itself is marked hot or cold.
> 
> for  gcc/testsuite/ChangeLog
> 
>      * g++.dg/ext/attr-hotness.C: New.
> 
> 
> Signed-off-by: Javier Martinez <javier.martinez.bugzilla@gmail.com>
> 
> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> index dc9579c..815df66 100644
> --- a/gcc/c-family/c-attribs.c
> +++ b/gcc/c-family/c-attribs.c
> @@ -398,10 +398,10 @@ const struct attribute_spec
> c_common_attribute_table[] =
>     { "alloc_size",      1, 2, false, true, true, false,
>         handle_alloc_size_attribute,
>                         attr_alloc_exclusions },
> -  { "cold",                   0, 0, true,  false, false, false,
> +  { "cold",                   0, 0, false,  false, false, false,
>         handle_cold_attribute,
>                         attr_cold_hot_exclusions },
> -  { "hot",                    0, 0, true,  false, false, false,
> +  { "hot",                    0, 0, false,  false, false, false,
>         handle_hot_attribute,
>                         attr_cold_hot_exclusions },
>     { "no_address_safety_analysis",
> @@ -837,22 +837,23 @@ handle_noreturn_attribute (tree *node, tree name,
> tree ARG_UNUSED (args),
> 
>   static tree
>   handle_hot_attribute (tree *node, tree name, tree ARG_UNUSED (args),
> -      int ARG_UNUSED (flags), bool *no_add_attrs)
> +       int ARG_UNUSED (flags), bool *no_add_attrs)

Try to avoid unnecessary whitespace changes.

>   {
> -  if (TREE_CODE (*node) == FUNCTION_DECL
> -      || TREE_CODE (*node) == LABEL_DECL)
> +  if ( (TREE_CODE (*node) == FUNCTION_DECL ||
> +        TREE_CODE (*node) == LABEL_DECL)
> +      || ((TREE_CODE(*node) == RECORD_TYPE ||
> +           TREE_CODE(*node) == UNION_TYPE) && c_dialect_cxx()))

In GCC we generally put a space before '(', not after it.
https://www.gnu.org/prep/standards/html_node/Formatting.html#Formatting

>       {
>         /* Attribute hot processing is done later with lookup_attribute.  */
>       }
>     else
>       {
>         warning (OPT_Wattributes, "%qE attribute ignored", name);
> -      *no_add_attrs = true;
> +      *no_add_attrs = true;

More unnecessary reformatting.

>       }
> 
>     return NULL_TREE;
>   }
> -

We want to keep this newline between this function and the comment for 
the next function.

>   /* Handle a "cold" and attribute; arguments as in
>      struct attribute_spec.handler.  */
> 
> @@ -860,15 +861,17 @@ static tree
>   handle_cold_attribute (tree *node, tree name, tree ARG_UNUSED (args),
>          int ARG_UNUSED (flags), bool *no_add_attrs)
>   {
> -  if (TREE_CODE (*node) == FUNCTION_DECL
> -      || TREE_CODE (*node) == LABEL_DECL)
> +  if ( (TREE_CODE (*node) == FUNCTION_DECL ||
> +        TREE_CODE (*node) == LABEL_DECL)
> +      || ((TREE_CODE(*node) == RECORD_TYPE ||
> +           TREE_CODE(*node) == UNION_TYPE) && c_dialect_cxx()))

As above.

>       {
>         /* Attribute cold processing is done later with lookup_attribute.  */
>       }
>     else
>       {
>         warning (OPT_Wattributes, "%qE attribute ignored", name);
> -      *no_add_attrs = true;
> +      *no_add_attrs = true;

As above.

>       }
> 
>     return NULL_TREE;
> diff --git a/gcc/cp/class.c b/gcc/cp/class.c
> index 07abe52..70f734f 100644
> --- a/gcc/cp/class.c
> +++ b/gcc/cp/class.c
> @@ -7540,6 +7540,35 @@ finish_struct (tree t, tree attributes)
>         && !LAMBDA_TYPE_P (t))
>       add_stmt (build_min (TAG_DEFN, t));
> 
> +

Unnecessary extra blank line.

> +  /* classes marked with hotness attributes propagate the attribute to

Capitalize the beginning of the comment.

> +  all methods. We propagate these here as there is a guarantee that
> +  TYPE_FIELDS is populated, as opposed from within decl_attributes. */
> +
> +  tree has_cold_attr = lookup_attribute("cold", TYPE_ATTRIBUTES(t));
> +  tree has_hot_attr = lookup_attribute("hot", TYPE_ATTRIBUTES(t));
> +
> +  if ( has_cold_attr || has_hot_attr ) {
> +
> +    /* hoisted out of the loop */
> +    tree attr_cold_id = get_identifier("cold");
> +    tree attr_hot_id = get_identifier("hot");

More space before parenthesis.

> +
> +    for (tree f = TYPE_FIELDS (t); f; f = DECL_CHAIN (f))
> +      {
> +        if (TREE_CODE (f) == FUNCTION_DECL) {
> +          /* decl_attributes will take care of conflicts,
> +          also prioritizing attributes explicitly marked in methods */
> +
> +          if (has_cold_attr) {
> +            decl_attributes (&f, tree_cons (attr_cold_id, NULL, NULL), 0);
> +          } else if (has_hot_attr) {
> +            decl_attributes (&f, tree_cons (attr_hot_id, NULL, NULL), 0);
> +          }

We also conventionally put the { for an if on a new line, and omit the { 
} for single substatements.

I think doing this here will miss lazily-declared special member 
functions; I wonder if you want to copy the attribute in grokclassfn 
instead?

> diff --git a/gcc/testsuite/g++.dg/ext/attr-hotness.C
> b/gcc/testsuite/g++.dg/ext/attr-hotness.C
> new file mode 100644
> index 0000000..075c624
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/ext/attr-hotness.C
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O0 -Wattributes -fdump-tree-gimple" } */
> +
> +
> +struct __attribute((cold)) A { __attribute((noinline, used)) void
> foo(void) { } };
> +
> +struct __attribute((cold)) B { __attribute((noinline, used, hot)) void
> foo(void) { } }; /* { dg-warning "ignoring attribute .cold. because it
> conflicts with attribute .hot." } */

Do we really want a warning for this case?  I'd expect the function 
attribute to take priority silently.

> +
> +struct __attribute((hot)) D { __attribute((noinline, used)) void foo(void)
> { } };
> +
> +struct __attribute((hot)) E { __attribute((noinline, used, cold)) void
> foo(void) { } }; /* { dg-warning "ignoring attribute .hot. because it
> conflicts with attribute .cold." } */

Likewise.

> +
> +/* { dg-final { scan-tree-dump-times "cold" 2 "gimple" } } */
> +/* { dg-final { scan-tree-dump-times "hot" 2 "gimple" } } */
> +
>