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.bugzi...@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" } } */
+
