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

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

[Javier Martinez]

[-- Attachment #1: Type: text/plain, Size: 10708 bytes --]

Thanks for the comments, Jason.
v2: + Fix formatting, remove unnecessarily warning.

On Tue, Aug 8, 2023 at 10:28 PM Jason Merrill <jason@redhat.com> wrote:
> Seems reasonable, but how do you expect this to be used?
We have large applications where some large classes are known to be used
only at startup. Marking every member function as cold is impractical so we
would mark those classes as cold.

> I think doing this here will miss lazily-declared special member
> functions; I wonder if you want to copy the attribute in grokclassfn
> instead?
I think they are being handled correctly with the current patch. Considered
the following program:

class __attribute((cold)) Foo {
public:
    int m_x, m_y;
    auto operator<=>(const Foo&) const = default;
};

int main(void) {
    Foo a{1,1};
    Foo b{1,2};

    std::set<Foo> s; // OK
    s.insert(a);     // OK

    std::cout << std::boolalpha
        << (a == b) << ' '
        << (a != b) << ' '
        << (a <  b) << ' '
        << (a <= b) << ' '
        << (a >  b) << ' '
        << (a >= b) << ' '
        << std::endl;
}

Per the rules for any operator<=> overload, a defaulted <=> overload will
also allow the type to be compared with <, <=, >, and >=. If operator<=> is
defaulted and operator== is not declared at all, then operator== is
implicitly defaulted.
The GIMPLE dump shows:
> __attribute__((cold))
> struct strong_ordering Foo::operator<=> (const struct Foo * const this,
const struct Foo & D.64195)
> __attribute__((cold))
> bool Foo::operator== (const struct Foo * const this, const struct Foo &
D.64206)

I think this makes sense as add_implicitly_declared_members is called
before my injection via finish_struct_1 -> check_bases_and_members.

---

I would like some comment on the implications of:
-  { "cold",       0, 0, true,  false, false, false,
+  { "cold",      0, 0, false,  false, false, false,

As I am now introducing a new warning that I cannot explain in an old test:
testsuite/g++.dg/Wmissing-attributes.C:55:22: warning: 'hot' attribute
ignored [-Wattributes]

> template <>
> void*
> ATTR ((hot)) ATTR ((alloc_size (1))) ATTR ((malloc))
> missing_all<char>(int);   // T = char, same as above

# Comparing 8 common sum files
## /bin/sh contrib/compare_tests  /tmp/gxx-sum1.948624 /tmp/gxx-sum2.948624
Tests that now fail, but worked before (4 tests):

g++.dg/Wmissing-attributes.C  -std=gnu++14 (test for excess errors)
g++.dg/Wmissing-attributes.C  -std=gnu++17 (test for excess errors)
g++.dg/Wmissing-attributes.C  -std=gnu++20 (test for excess errors)
g++.dg/Wmissing-attributes.C  -std=gnu++98 (test for excess errors)

New tests that PASS (20 tests):

g++.dg/ext/attr-hotness.C  -std=gnu++14  (test for warnings, line 11)
g++.dg/ext/attr-hotness.C  -std=gnu++14  (test for warnings, line 9)
g++.dg/ext/attr-hotness.C  -std=gnu++14  scan-tree-dump-times gimple "cold"
2
g++.dg/ext/attr-hotness.C  -std=gnu++14  scan-tree-dump-times gimple "hot" 2
g++.dg/ext/attr-hotness.C  -std=gnu++14 (test for excess errors)
g++.dg/ext/attr-hotness.C  -std=gnu++17  (test for warnings, line 11)
g++.dg/ext/attr-hotness.C  -std=gnu++17  (test for warnings, line 9)
g++.dg/ext/attr-hotness.C  -std=gnu++17  scan-tree-dump-times gimple "cold"
2
g++.dg/ext/attr-hotness.C  -std=gnu++17  scan-tree-dump-times gimple "hot" 2
g++.dg/ext/attr-hotness.C  -std=gnu++17 (test for excess errors)
g++.dg/ext/attr-hotness.C  -std=gnu++20  (test for warnings, line 11)
g++.dg/ext/attr-hotness.C  -std=gnu++20  (test for warnings, line 9)
g++.dg/ext/attr-hotness.C  -std=gnu++20  scan-tree-dump-times gimple "cold"
2
g++.dg/ext/attr-hotness.C  -std=gnu++20  scan-tree-dump-times gimple "hot" 2
g++.dg/ext/attr-hotness.C  -std=gnu++20 (test for excess errors)
g++.dg/ext/attr-hotness.C  -std=gnu++98  (test for warnings, line 11)
g++.dg/ext/attr-hotness.C  -std=gnu++98  (test for warnings, line 9)
g++.dg/ext/attr-hotness.C  -std=gnu++98  scan-tree-dump-times gimple "cold"
2
g++.dg/ext/attr-hotness.C  -std=gnu++98  scan-tree-dump-times gimple "hot" 2
g++.dg/ext/attr-hotness.C  -std=gnu++98 (test for excess errors)

## Differences found:
# 1 differences in 8 common sum files found

---

gcc/c-family/ChangeLog:

        * c-attribs.cc (handle_hot_attribute): remove warning on RECORD_TYPE
        and UNION_TYPE when in c_dialect_xx.
        (handle_cold_attribute): Likewise.

gcc/cp/ChangeLog:

        * class.cc (propagate_class_warmth_attribute): New function.
        (finish_struct): propagate hot and cold
        attributes to all FUNCTION_DECL when the class is marked hot or
cold.

gcc/testsuite/ChangeLog:

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

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

---
 gcc/c-family/c-attribs.cc               | 26 ++++++++++++--
 gcc/cp/class.cc                         | 47 +++++++++++++++++++++++++
 gcc/testsuite/g++.dg/ext/attr-hotness.C | 16 +++++++++
 3 files changed, 87 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/ext/attr-hotness.C

diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
index e2792ca6898..090108ad94c 100644
--- a/gcc/c-family/c-attribs.cc
+++ b/gcc/c-family/c-attribs.cc
@@ -452,10 +452,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",
@@ -1110,6 +1110,17 @@ handle_hot_attribute (tree *node, tree name, tree
ARG_UNUSED (args),
     {
       /* Attribute hot processing is done later with lookup_attribute.  */
     }
+  else if ((TREE_CODE (*node) == RECORD_TYPE
+    || TREE_CODE (*node) == UNION_TYPE)
+  && c_dialect_cxx ())
+    {
+      /* Check conflict here as decl_attributes will otherwise only catch
+ it late at the function when the attribute is used on a class.  */
+      tree cold_attr = lookup_attribute ("cold", TYPE_ATTRIBUTES (*node));
+      if (cold_attr)
+ warning (OPT_Wattributes, "ignoring attribute %qE because it "
+ "conflicts with attribute %qs", name, "cold");
+    }
   else
     {
       warning (OPT_Wattributes, "%qE attribute ignored", name);
@@ -1131,6 +1142,17 @@ handle_cold_attribute (tree *node, tree name, tree
ARG_UNUSED (args),
     {
       /* Attribute cold processing is done later with lookup_attribute.  */
     }
+  else if ((TREE_CODE (*node) == RECORD_TYPE
+    || TREE_CODE (*node) == UNION_TYPE)
+  && c_dialect_cxx ())
+    {
+      /* Check conflict here as decl_attributes will otherwise only catch
+ it late at the function when the attribute is used on a class.  */
+      tree hot_attr = lookup_attribute ("hot", TYPE_ATTRIBUTES (*node));
+      if (hot_attr)
+ warning (OPT_Wattributes, "ignoring attribute %qE because it "
+    "conflicts with attribute %qs", name, "hot");
+    }
   else
     {
       warning (OPT_Wattributes, "%qE attribute ignored", name);
diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index 778759237dc..9543ad69cb1 100644
--- a/gcc/cp/class.cc
+++ b/gcc/cp/class.cc
@@ -205,6 +205,7 @@ static tree get_vcall_index (tree, tree);
 static bool type_maybe_constexpr_default_constructor (tree);
 static bool type_maybe_constexpr_destructor (tree);
 static bool field_poverlapping_p (tree);
+static void propagate_class_warmth_attribute (tree);

 /* Set CURRENT_ACCESS_SPECIFIER based on the protection of DECL.  */

@@ -7733,6 +7734,48 @@ unreverse_member_declarations (tree t)
     }
 }

+/* Classes marked with hotness attributes propagate the attribute to
+   all methods.  */
+void
+propagate_class_warmth_attribute (tree t)
+{
+  tree class_has_cold_attr = lookup_attribute ("cold",
+   TYPE_ATTRIBUTES (t));
+  tree class_has_hot_attr = lookup_attribute ("hot",
+   TYPE_ATTRIBUTES (t));
+
+  if (class_has_cold_attr || class_has_hot_attr)
+    {
+      tree attr_cold_id = get_identifier ("cold");
+      tree attr_hot_id = get_identifier ("hot");
+
+      for (tree f = TYPE_FIELDS (t); f; f = DECL_CHAIN (f))
+ if (TREE_CODE (f) == FUNCTION_DECL)
+  {
+
+    /* Transparently ignore the new warmth attribute if it
+       conflicts with a present function attribute.  Otherwise
+       decl_attribute would still honour the present attribute,
+       but producing an undesired warning in the process.  */
+
+    if (class_has_cold_attr)
+      {
+ if (lookup_attribute ("hot",
+ DECL_ATTRIBUTES (f)) == NULL)
+  decl_attributes (&f,
+ tree_cons (attr_cold_id, NULL, NULL), 0);
+      }
+    else if (class_has_hot_attr)
+      {
+ if (lookup_attribute ("cold",
+ DECL_ATTRIBUTES (f)) == NULL)
+  decl_attributes (&f,
+ tree_cons (attr_hot_id, NULL, NULL), 0);
+      }
+  }
+    }
+}
+
 tree
 finish_struct (tree t, tree attributes)
 {
@@ -7866,6 +7909,10 @@ finish_struct (tree t, tree attributes)
       && !LAMBDA_TYPE_P (t))
     add_stmt (build_min (TAG_DEFN, t));

+  /* This must be done after all lazily declared special member funcitons
+     have been injected.  */
+  propagate_class_warmth_attribute (t);
+
   return t;
 }

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 00000000000..f9a6930304d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ext/attr-hotness.C
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-O0 -Wattributes -fdump-tree-gimple" } */
+
+
+struct __attribute((cold)) A { __attribute((noinline, used)) void
foo(void) { } };
+
+struct __attribute((hot)) B { __attribute((noinline, used)) void foo(void)
{ } };
+
+struct __attribute((hot, cold)) C { __attribute((noinline, used)) void
foo(void) { } }; /* { dg-warning "ignoring attribute .cold. because it
conflicts with attribute .hot." } */
+
+struct __attribute((cold, hot)) D { __attribute((noinline, used)) void
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" } } */
+
-- 
2.34.1

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

[Jason Merrill]

On 8/10/23 14:06, Javier Martinez wrote:
> 
> Thanks for the comments, Jason.
> v2: + Fix formatting, remove unnecessarily warning.
> 
> On Tue, Aug 8, 2023 at 10:28 PM Jason Merrill <jason@redhat.com 
> <mailto:jason@redhat.com>> wrote:
>  > Seems reasonable, but how do you expect this to be used?
> We have large applications where some large classes are known to be used 
> only at startup. Marking every member function as cold is impractical so 
> we would mark those classes as cold.

Sure, though I think of the cold attribute as being primarily to mark 
unlikely paths in otherwise hot code; is this a

if (!initialized)
   do_initialization();

kind of situation?

>  > I think doing this here will miss lazily-declared special member
>  > functions; I wonder if you want to copy the attribute in grokclassfn
>  > instead?
> I think they are being handled correctly with the current patch. 
> Considered the following program:
> 
> class __attribute((cold)) Foo {
> public:
>      int m_x, m_y;
>      auto operator<=>(const Foo&) const = default;
> };
> 
> int main(void) {
>      Foo a{1,1};
>      Foo b{1,2};
> 
>      std::set<Foo> s; // OK
>      s.insert(a);     // OK
> 
>      std::cout << std::boolalpha
>          << (a == b) << ' '
>          << (a != b) << ' '
>          << (a <  b) << ' '
>          << (a <= b) << ' '
>          << (a >  b) << ' '
>          << (a >= b) << ' '
>          << std::endl;
> }
> 
> Per the rules for any operator<=> overload, a defaulted <=> overload 
> will also allow the type to be compared with <, <=, >, and >=. If 
> operator<=> is defaulted and operator== is not declared at all, then 
> operator== is implicitly defaulted.
> The GIMPLE dump shows:
>  > __attribute__((cold))
>  > struct strong_ordering Foo::operator<=> (const struct Foo * const 
> this, const struct Foo & D.64195)
>  > __attribute__((cold))
>  > bool Foo::operator== (const struct Foo * const this, const struct Foo 
> & D.64206)
> 
> I think this makes sense as add_implicitly_declared_members is called 
> before my injection via finish_struct_1 -> check_bases_and_members.

Yes, but that's because the implicit op== isn't declared lazily like 
some other special member functions (CLASSTYPE_LAZY_*/lazily_declare_fn) 
which can happen after the class is complete.

> ---
> 
> I would like some comment on the implications of:
> -  { "cold",       0, 0, true,  false, false, false,
> +  { "cold",      0, 0, false,  false, false, false,
> 
> As I am now introducing a new warning that I cannot explain in an old test:
> testsuite/g++.dg/Wmissing-attributes.C:55:22: warning: 'hot' attribute 
> ignored [-Wattributes]
> 
>  > template <>
>  > void*
>  > ATTR ((hot)) ATTR ((alloc_size (1))) ATTR ((malloc))
>  > missing_all<char>(int);   // T = char, same as above

I think this is because attributes in that position appertain to the 
return type rather than the declaration.  At attribs.cc:753, if an 
attribute has decl_required true and we try to apply it to a return 
type, we pass it along to the declaration instead; if you change the 
true to false we will try to apply it to void* directly and fail.

I think it would work to check for (flags & (ATTR_FLAG_FUNCTION_NEXT | 
ATTR_FLAG_DECL_NEXT)) and return without warning in that case.  You'd 
still set *no_add_attr.

BTW the patch got garbled this time, probably safer to make it an 
attachment if you're using gmail.

Jason