Add -Wsuggest-attribute=cold

Add -Wsuggest-attribute=cold

[Jan Hubicka]

Hi,
this patch adds -Wsuggest-attribute=cold because we can now statically detect
cold functions atuomatically.

Bootstrapped/regtested x86_64-linux. Plan to commit it tomorrow if there are no
complains.

Honza

	* invoke.texi (Wsuggest-attribute=cold): Document.
	* common.opt (Wsuggest-attribute=cold): New
	* ipa-pure-const.c (warn_function_cold): New function.
	* predict.c (compute_function_frequency): Use it.
	* predict.h (warn_function_cold): Declare.

	* gcc.dg/cold-1.c: New testcase.
Index: common.opt
===================================================================
--- common.opt	(revision 250470)
+++ common.opt	(working copy)
@@ -708,6 +708,10 @@ Wstrict-overflow=
 Common Joined RejectNegative UInteger Var(warn_strict_overflow) Warning
 Warn about optimizations that assume that signed overflow is undefined.
 
+Wsuggest-attribute=cold
+Common Var(warn_suggest_attribute_cold) Warning
+Warn about functions which might be candidates for __attribute__((cold)).
+
 Wsuggest-attribute=const
 Common Var(warn_suggest_attribute_const) Warning
 Warn about functions which might be candidates for __attribute__((const)).
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 250470)
+++ doc/invoke.texi	(working copy)
@@ -5172,7 +5172,7 @@ whether to issue a warning.  Similarly t
 setting of the option may result in warnings for benign code.
 @end table
 
-@item -Wsuggest-attribute=@r{[}pure@r{|}const@r{|}noreturn@r{|}format@r{]}
+@item -Wsuggest-attribute=@r{[}pure@r{|}const@r{|}noreturn@r{|}format@r{|}cold@r{]}
 @opindex Wsuggest-attribute=
 @opindex Wno-suggest-attribute=
 Warn for cases where adding an attribute may be beneficial. The
@@ -5224,6 +5224,15 @@ might be appropriate for any function th
 @code{vprintf} or @code{vscanf}, but this might not always be the
 case, and some functions for which @code{format} attributes are
 appropriate may not be detected.
+
+@item -Wsuggest-attribute=cold
+@opindex Wsuggest-attribute=cold
+@opindex Wno-suggest-attribute=cold
+
+Warn about functions that might be candidates for @code{cold} attribute.  This
+is based on static detection and generally will only warn about functions which
+always leads to a call to another @code{cold} function such as wrappers of
+C++ @code{throw} or fatal error reporting functions leading to @code{abort}.
 @end table
 
 @item -Wsuggest-final-types
Index: ipa-pure-const.c
===================================================================
--- ipa-pure-const.c	(revision 250470)
+++ ipa-pure-const.c	(working copy)
@@ -232,6 +233,21 @@ warn_function_noreturn (tree decl)
 			   true, warned_about, "noreturn");
 }
 
+void
+warn_function_cold (tree decl)
+{
+  tree original_decl = decl;
+
+  cgraph_node *node = cgraph_node::get (decl);
+  if (node->instrumentation_clone)
+    decl = node->instrumented_version->decl;
+
+  static hash_set<tree> *warned_about;
+  warned_about 
+    = suggest_attribute (OPT_Wsuggest_attribute_cold, original_decl,
+			 true, warned_about, "cold");
+}
+
 /* Return true if we have a function state for NODE.  */
 
 static inline bool
Index: predict.c
===================================================================
--- predict.c	(revision 250470)
+++ predict.c	(working copy)
@@ -3611,7 +3611,10 @@ compute_function_frequency (void)
       if (ENTRY_BLOCK_PTR_FOR_FN (cfun)->count == profile_count::zero ()
 	  || lookup_attribute ("cold", DECL_ATTRIBUTES (current_function_decl))
 	     != NULL)
-        node->frequency = NODE_FREQUENCY_UNLIKELY_EXECUTED;
+	{
+          node->frequency = NODE_FREQUENCY_UNLIKELY_EXECUTED;
+	  warn_function_cold (current_function_decl);
+	}
       else if (lookup_attribute ("hot", DECL_ATTRIBUTES (current_function_decl))
 	       != NULL)
         node->frequency = NODE_FREQUENCY_HOT;
@@ -3630,7 +3633,10 @@ compute_function_frequency (void)
      Ipa-profile pass will drop functions only called from unlikely
      functions to unlikely and that is most of what we care about.  */
   if (!cfun->after_inlining)
-    node->frequency = NODE_FREQUENCY_UNLIKELY_EXECUTED;
+    {
+      node->frequency = NODE_FREQUENCY_UNLIKELY_EXECUTED;
+      warn_function_cold (current_function_decl);
+    }
   FOR_EACH_BB_FN (bb, cfun)
     {
       if (maybe_hot_bb_p (cfun, bb))
Index: predict.h
===================================================================
--- predict.h	(revision 250470)
+++ predict.h	(working copy)
@@ -102,4 +102,7 @@ extern void propagate_unlikely_bbs_forwa
 
 extern void add_reg_br_prob_note (rtx_insn *, profile_probability);
 
+/* In ipa-pure-const.c   */
+extern void warn_function_cold (tree);
+
 #endif  /* GCC_PREDICT_H */
Index: testsuite/gcc.dg/cold-1.c
===================================================================
--- testsuite/gcc.dg/cold-1.c	(revision 0)
+++ testsuite/gcc.dg/cold-1.c	(working copy)
@@ -0,0 +1,21 @@
+/* { dg-do compile { target nonpic } } */
+/* { dg-options "-O2 -Wsuggest-attribute=cold" } */
+
+extern void do_something_interesting_and_never_return ();
+
+int
+foo1(int a)  
+{ /* { dg-warning "cold" "detect cold candidate" { target *-*-* } "8" } */ 
+  if (a)
+    abort ();
+  else
+    abort ();
+}
+
+int
+foo2(int a)  
+{ 
+  if (a)
+    do_something_interesting_and_never_return ();
+  abort ();
+}

Re: Add -Wsuggest-attribute=cold

[Eric Gallager]

On Mon, Jul 24, 2017 at 2:56 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> this patch adds -Wsuggest-attribute=cold because we can now statically detect
> cold functions atuomatically.
>
> Bootstrapped/regtested x86_64-linux. Plan to commit it tomorrow if there are no
> complains.
>
> Honza
>
>         * invoke.texi (Wsuggest-attribute=cold): Document.
>         * common.opt (Wsuggest-attribute=cold): New
>         * ipa-pure-const.c (warn_function_cold): New function.
>         * predict.c (compute_function_frequency): Use it.
>         * predict.h (warn_function_cold): Declare.
>
>         * gcc.dg/cold-1.c: New testcase.

Would it be possible to also do -Wsuggest-attribute=hot for symmetry's
sake? Just wondering.

> Index: common.opt
> ===================================================================
> --- common.opt  (revision 250470)
> +++ common.opt  (working copy)
> @@ -708,6 +708,10 @@ Wstrict-overflow=
>  Common Joined RejectNegative UInteger Var(warn_strict_overflow) Warning
>  Warn about optimizations that assume that signed overflow is undefined.
>
> +Wsuggest-attribute=cold
> +Common Var(warn_suggest_attribute_cold) Warning
> +Warn about functions which might be candidates for __attribute__((cold)).
> +
>  Wsuggest-attribute=const
>  Common Var(warn_suggest_attribute_const) Warning
>  Warn about functions which might be candidates for __attribute__((const)).
> Index: doc/invoke.texi
> ===================================================================
> --- doc/invoke.texi     (revision 250470)
> +++ doc/invoke.texi     (working copy)
> @@ -5172,7 +5172,7 @@ whether to issue a warning.  Similarly t
>  setting of the option may result in warnings for benign code.
>  @end table
>
> -@item -Wsuggest-attribute=@r{[}pure@r{|}const@r{|}noreturn@r{|}format@r{]}
> +@item -Wsuggest-attribute=@r{[}pure@r{|}const@r{|}noreturn@r{|}format@r{|}cold@r{]}
>  @opindex Wsuggest-attribute=
>  @opindex Wno-suggest-attribute=
>  Warn for cases where adding an attribute may be beneficial. The
> @@ -5224,6 +5224,15 @@ might be appropriate for any function th
>  @code{vprintf} or @code{vscanf}, but this might not always be the
>  case, and some functions for which @code{format} attributes are
>  appropriate may not be detected.
> +
> +@item -Wsuggest-attribute=cold
> +@opindex Wsuggest-attribute=cold
> +@opindex Wno-suggest-attribute=cold
> +
> +Warn about functions that might be candidates for @code{cold} attribute.  This
> +is based on static detection and generally will only warn about functions which
> +always leads to a call to another @code{cold} function such as wrappers of
> +C++ @code{throw} or fatal error reporting functions leading to @code{abort}.
>  @end table
>
>  @item -Wsuggest-final-types
> Index: ipa-pure-const.c
> ===================================================================
> --- ipa-pure-const.c    (revision 250470)
> +++ ipa-pure-const.c    (working copy)
> @@ -232,6 +233,21 @@ warn_function_noreturn (tree decl)
>                            true, warned_about, "noreturn");
>  }
>
> +void
> +warn_function_cold (tree decl)
> +{
> +  tree original_decl = decl;
> +
> +  cgraph_node *node = cgraph_node::get (decl);
> +  if (node->instrumentation_clone)
> +    decl = node->instrumented_version->decl;
> +
> +  static hash_set<tree> *warned_about;
> +  warned_about
> +    = suggest_attribute (OPT_Wsuggest_attribute_cold, original_decl,
> +                        true, warned_about, "cold");
> +}
> +
>  /* Return true if we have a function state for NODE.  */
>
>  static inline bool
> Index: predict.c
> ===================================================================
> --- predict.c   (revision 250470)
> +++ predict.c   (working copy)
> @@ -3611,7 +3611,10 @@ compute_function_frequency (void)
>        if (ENTRY_BLOCK_PTR_FOR_FN (cfun)->count == profile_count::zero ()
>           || lookup_attribute ("cold", DECL_ATTRIBUTES (current_function_decl))
>              != NULL)
> -        node->frequency = NODE_FREQUENCY_UNLIKELY_EXECUTED;
> +       {
> +          node->frequency = NODE_FREQUENCY_UNLIKELY_EXECUTED;
> +         warn_function_cold (current_function_decl);
> +       }
>        else if (lookup_attribute ("hot", DECL_ATTRIBUTES (current_function_decl))
>                != NULL)
>          node->frequency = NODE_FREQUENCY_HOT;
> @@ -3630,7 +3633,10 @@ compute_function_frequency (void)
>       Ipa-profile pass will drop functions only called from unlikely
>       functions to unlikely and that is most of what we care about.  */
>    if (!cfun->after_inlining)
> -    node->frequency = NODE_FREQUENCY_UNLIKELY_EXECUTED;
> +    {
> +      node->frequency = NODE_FREQUENCY_UNLIKELY_EXECUTED;
> +      warn_function_cold (current_function_decl);
> +    }
>    FOR_EACH_BB_FN (bb, cfun)
>      {
>        if (maybe_hot_bb_p (cfun, bb))
> Index: predict.h
> ===================================================================
> --- predict.h   (revision 250470)
> +++ predict.h   (working copy)
> @@ -102,4 +102,7 @@ extern void propagate_unlikely_bbs_forwa
>
>  extern void add_reg_br_prob_note (rtx_insn *, profile_probability);
>
> +/* In ipa-pure-const.c   */
> +extern void warn_function_cold (tree);
> +
>  #endif  /* GCC_PREDICT_H */
> Index: testsuite/gcc.dg/cold-1.c
> ===================================================================
> --- testsuite/gcc.dg/cold-1.c   (revision 0)
> +++ testsuite/gcc.dg/cold-1.c   (working copy)
> @@ -0,0 +1,21 @@
> +/* { dg-do compile { target nonpic } } */
> +/* { dg-options "-O2 -Wsuggest-attribute=cold" } */
> +
> +extern void do_something_interesting_and_never_return ();
> +
> +int
> +foo1(int a)
> +{ /* { dg-warning "cold" "detect cold candidate" { target *-*-* } "8" } */
> +  if (a)
> +    abort ();
> +  else
> +    abort ();
> +}
> +
> +int
> +foo2(int a)
> +{
> +  if (a)
> +    do_something_interesting_and_never_return ();
> +  abort ();
> +}

Re: Add -Wsuggest-attribute=cold

[Jan Hubicka]

> On Mon, Jul 24, 2017 at 2:56 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> > Hi,
> > this patch adds -Wsuggest-attribute=cold because we can now statically detect
> > cold functions atuomatically.
> >
> > Bootstrapped/regtested x86_64-linux. Plan to commit it tomorrow if there are no
> > complains.
> >
> > Honza
> >
> >         * invoke.texi (Wsuggest-attribute=cold): Document.
> >         * common.opt (Wsuggest-attribute=cold): New
> >         * ipa-pure-const.c (warn_function_cold): New function.
> >         * predict.c (compute_function_frequency): Use it.
> >         * predict.h (warn_function_cold): Declare.
> >
> >         * gcc.dg/cold-1.c: New testcase.
> 
> Would it be possible to also do -Wsuggest-attribute=hot for symmetry's
> sake? Just wondering.

It would be nice, but it is kind of impossible to detect hot spots of the
program with reasonable certainity. (that is why profile feedback is useful :)
This cold attribute detection looks really for very simple pattern where the
function inavoidably calls other cold function or does something similarly
unlikely (Eh or trap).  This is fairly limited pattern, but it is useful i.e.
to detect which libstdc++ functions can have this annotation that further
improve branch prediction.

Honza

Re: Add -Wsuggest-attribute=cold

[Jeff Law]

On 07/24/2017 01:08 PM, Jan Hubicka wrote:
>> On Mon, Jul 24, 2017 at 2:56 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>> Hi,
>>> this patch adds -Wsuggest-attribute=cold because we can now statically detect
>>> cold functions atuomatically.
>>>
>>> Bootstrapped/regtested x86_64-linux. Plan to commit it tomorrow if there are no
>>> complains.
>>>
>>> Honza
>>>
>>>         * invoke.texi (Wsuggest-attribute=cold): Document.
>>>         * common.opt (Wsuggest-attribute=cold): New
>>>         * ipa-pure-const.c (warn_function_cold): New function.
>>>         * predict.c (compute_function_frequency): Use it.
>>>         * predict.h (warn_function_cold): Declare.
>>>
>>>         * gcc.dg/cold-1.c: New testcase.
>>
>> Would it be possible to also do -Wsuggest-attribute=hot for symmetry's
>> sake? Just wondering.
> 
> It would be nice, but it is kind of impossible to detect hot spots of the
> program with reasonable certainity. (that is why profile feedback is useful :)
> This cold attribute detection looks really for very simple pattern where the
> function inavoidably calls other cold function or does something similarly
> unlikely (Eh or trap).  This is fairly limited pattern, but it is useful i.e.
> to detect which libstdc++ functions can have this annotation that further
> improve branch prediction.
So what's the advantage of a user adding the attribute if the compiler
can infer it?  Presumably by adding the attribute, it's known without
analysis and can be taken advantage of by its callers?
Jeff

Re: Add -Wsuggest-attribute=cold

[Jan Hubicka]

> On 07/24/2017 01:08 PM, Jan Hubicka wrote:
> >> On Mon, Jul 24, 2017 at 2:56 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> >>> Hi,
> >>> this patch adds -Wsuggest-attribute=cold because we can now statically detect
> >>> cold functions atuomatically.
> >>>
> >>> Bootstrapped/regtested x86_64-linux. Plan to commit it tomorrow if there are no
> >>> complains.
> >>>
> >>> Honza
> >>>
> >>>         * invoke.texi (Wsuggest-attribute=cold): Document.
> >>>         * common.opt (Wsuggest-attribute=cold): New
> >>>         * ipa-pure-const.c (warn_function_cold): New function.
> >>>         * predict.c (compute_function_frequency): Use it.
> >>>         * predict.h (warn_function_cold): Declare.
> >>>
> >>>         * gcc.dg/cold-1.c: New testcase.
> >>
> >> Would it be possible to also do -Wsuggest-attribute=hot for symmetry's
> >> sake? Just wondering.
> > 
> > It would be nice, but it is kind of impossible to detect hot spots of the
> > program with reasonable certainity. (that is why profile feedback is useful :)
> > This cold attribute detection looks really for very simple pattern where the
> > function inavoidably calls other cold function or does something similarly
> > unlikely (Eh or trap).  This is fairly limited pattern, but it is useful i.e.
> > to detect which libstdc++ functions can have this annotation that further
> > improve branch prediction.
> So what's the advantage of a user adding the attribute if the compiler
> can infer it?  Presumably by adding the attribute, it's known without
> analysis and can be taken advantage of by its callers?

Like most of the other -Wsuggest it only warn on functions that are externally
visble. Adding attribute will improve code in other compilation units that does
not see the function body.

Honza
> Jeff

Re: Add -Wsuggest-attribute=cold

[Jeff Law]

On 07/24/2017 04:06 PM, Jan Hubicka wrote:
>> On 07/24/2017 01:08 PM, Jan Hubicka wrote:
>>>> On Mon, Jul 24, 2017 at 2:56 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>>>> Hi,
>>>>> this patch adds -Wsuggest-attribute=cold because we can now statically detect
>>>>> cold functions atuomatically.
>>>>>
>>>>> Bootstrapped/regtested x86_64-linux. Plan to commit it tomorrow if there are no
>>>>> complains.
>>>>>
>>>>> Honza
>>>>>
>>>>>         * invoke.texi (Wsuggest-attribute=cold): Document.
>>>>>         * common.opt (Wsuggest-attribute=cold): New
>>>>>         * ipa-pure-const.c (warn_function_cold): New function.
>>>>>         * predict.c (compute_function_frequency): Use it.
>>>>>         * predict.h (warn_function_cold): Declare.
>>>>>
>>>>>         * gcc.dg/cold-1.c: New testcase.
>>>>
>>>> Would it be possible to also do -Wsuggest-attribute=hot for symmetry's
>>>> sake? Just wondering.
>>>
>>> It would be nice, but it is kind of impossible to detect hot spots of the
>>> program with reasonable certainity. (that is why profile feedback is useful :)
>>> This cold attribute detection looks really for very simple pattern where the
>>> function inavoidably calls other cold function or does something similarly
>>> unlikely (Eh or trap).  This is fairly limited pattern, but it is useful i.e.
>>> to detect which libstdc++ functions can have this annotation that further
>>> improve branch prediction.
>> So what's the advantage of a user adding the attribute if the compiler
>> can infer it?  Presumably by adding the attribute, it's known without
>> analysis and can be taken advantage of by its callers?
> 
> Like most of the other -Wsuggest it only warn on functions that are externally
> visble. Adding attribute will improve code in other compilation units that does
> not see the function body.
OK.  THanks for confirming.

Jeff

Re: Add -Wsuggest-attribute=cold

[Martin Sebor]

> +extern void do_something_interesting_and_never_return ();
> +
> +int
> +foo1(int a)
> +{ /* { dg-warning "cold" "detect cold candidate" { target *-*-* } "8" } */
> +  if (a)
> +    abort ();
> +  else
> +    abort ();
> +}

In this case it looks to me like with the patch GCC will actually
issue two suggestions: the new -Wsuggest-attribute=cold and the
existing -Wsuggest-attrribute=noreturn.

It's probably fine to have the same function annotated with both
attributes but I think it would be nice to give just one suggestion,
and recommend the optimal of the two (presumably noreturn).

Martin

> +
> +int
> +foo2(int a)
> +{
> +  if (a)
> +    do_something_interesting_and_never_return ();
> +  abort ();
> +}
>

Re: Add -Wsuggest-attribute=cold

[Tom de Vries]

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

On 07/24/2017 08:56 PM, Jan Hubicka wrote:
> 	* gcc.dg/cold-1.c: New testcase.

> Index: testsuite/gcc.dg/cold-1.c
> ===================================================================
> --- testsuite/gcc.dg/cold-1.c	(revision 0)
> +++ testsuite/gcc.dg/cold-1.c	(working copy)
> @@ -0,0 +1,21 @@
> +/* { dg-do compile { target nonpic } } */
> +/* { dg-options "-O2 -Wsuggest-attribute=cold" } */
> +
> +extern void do_something_interesting_and_never_return ();
> +
> +int
> +foo1(int a)
> +{ /* { dg-warning "cold" "detect cold candidate" { target *-*-* } "8" } */
> +  if (a)
> +    abort ();
> +  else
> +    abort ();
> +}
> +
> +int
> +foo2(int a)
> +{
> +  if (a)
> +    do_something_interesting_and_never_return ();
> +  abort ();
> +}
> 

Hi,

this test-case failed for me due to:
- excess errors due to missing abort declaration
- warning emitted on line 7, but is expected on line 8

This patch that:
- fixes the warning line number
- rewrites the absolute warning line number into a relative one
- adds the abort declaration

Committed as obvious.

Thanks,
- Tom

[-- Attachment #2: 0001-Fix-gcc.dg-cold-1.c.patch --]
[-- Type: text/x-patch, Size: 861 bytes --]

Fix gcc.dg/cold-1.c

2017-10-09  Tom de Vries  <tom@codesourcery.com>

	* gcc.dg/cold-1.c (foo1): Fix warning line number.  Make warning line
	number relative.
	(abort): Declare.

---
 gcc/testsuite/gcc.dg/cold-1.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/testsuite/gcc.dg/cold-1.c b/gcc/testsuite/gcc.dg/cold-1.c
index 8ea88dd..ba1cd3a 100644
--- a/gcc/testsuite/gcc.dg/cold-1.c
+++ b/gcc/testsuite/gcc.dg/cold-1.c
@@ -1,11 +1,12 @@
 /* { dg-do compile { target nonpic } } */
 /* { dg-options "-O2 -Wsuggest-attribute=cold" } */
 
+extern void abort (void);
 extern void do_something_interesting_and_never_return ();
 
 int
 foo1(int a)  
-{ /* { dg-warning "cold" "detect cold candidate" { target *-*-* } "8" } */ 
+{ /* { dg-warning "cold" "detect cold candidate" { target *-*-* } ".-1" } */
   if (a)
     abort ();
   else