| Message ID | 20170724185633.GA43541@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
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 (); > +}
> 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
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
> 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
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
> +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 (); > +} >
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 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
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 (); +}