| Message ID | 5011BD69.4080503@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jul 26, 2012 at 11:58 PM, Richard Henderson <rth@redhat.com> wrote: > On 07/26/2012 02:41 PM, Richard Henderson wrote: >> This is a patch... > > ... that I should have attached. Bah. Do we need to mark the labels so we preserve them? Consider goto foo; foo: bar __attribute__((cold)): ... so bar will be unused? What about BB merging if we end up with <BB 3>: .. fallthru bar __attribute__((cold)): ... should BB 3 inherit the coldness? I think we no longer disable BB merging if the destination has user labels. Richard.
On Fri, Jul 27, 2012 at 10:57 AM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Thu, Jul 26, 2012 at 11:58 PM, Richard Henderson <rth@redhat.com> wrote: >> On 07/26/2012 02:41 PM, Richard Henderson wrote: >>> This is a patch... >> >> ... that I should have attached. Bah. > > Do we need to mark the labels so we preserve them? Consider > > goto foo; > > foo: > bar __attribute__((cold)): > ... > > so bar will be unused? What about BB merging if we end up with > > <BB 3>: > .. > fallthru > bar __attribute__((cold)): > ... > > should BB 3 inherit the coldness? I think we no longer disable > BB merging if the destination has user labels. Right. I don't like the use of this attribute on labels at all, for the reasons you list here. I think it would be much cleaner to add a branch hint on the label in the asm goto, to contain this extension and to also to make it clear that it's not the label that is cold but the jump that is unlikely to be executed (i.e. cause and effect: the jump is unlikely and therefore the basic block is cold). Something like this: asm-goto-operands: asm-got-branch-hint identifier asm-goto-operands , asm-got-branch-hint identifier asm-got-branch-hint: <empty | + | -> where + means branch-likely and - means branch-unlikely -> asm goto (""::::+l1); asm goto (""::::+l1); asm goto (""::::-l1); Ciao! Steven
On Fri, Jul 27, 2012 at 11:08 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > On Fri, Jul 27, 2012 at 10:57 AM, Richard Guenther > <richard.guenther@gmail.com> wrote: >> On Thu, Jul 26, 2012 at 11:58 PM, Richard Henderson <rth@redhat.com> wrote: >>> On 07/26/2012 02:41 PM, Richard Henderson wrote: >>>> This is a patch... >>> >>> ... that I should have attached. Bah. >> >> Do we need to mark the labels so we preserve them? Consider >> >> goto foo; >> >> foo: >> bar __attribute__((cold)): >> ... >> >> so bar will be unused? What about BB merging if we end up with >> >> <BB 3>: >> .. >> fallthru >> bar __attribute__((cold)): >> ... >> >> should BB 3 inherit the coldness? I think we no longer disable >> BB merging if the destination has user labels. > > Right. I don't like the use of this attribute on labels at all, for > the reasons you list here. I think it would be much cleaner to add a > branch hint on the label in the asm goto, to contain this extension > and to also to make it clear that it's not the label that is cold but > the jump that is unlikely to be executed (i.e. cause and effect: the > jump is unlikely and therefore the basic block is cold). As in the case where you have both an unlikely and likely jump to a basic-block. But what I understand is that rth adds a way to mark a basic-block as hot or cold, not a way to mark an edge as hot or cold (that would be what the asm goto annotation would do). Both cases are of course useful. Richard. > Something like this: > > asm-goto-operands: > asm-got-branch-hint identifier > asm-goto-operands , asm-got-branch-hint identifier > > asm-got-branch-hint: <empty | + | -> > where + means branch-likely and - means branch-unlikely > > -> > > asm goto (""::::+l1); > asm goto (""::::+l1); > asm goto (""::::-l1); > Ciao! > Steven
On Fri, Jul 27, 2012 at 11:15 AM, Richard Guenther <richard.guenther@gmail.com> wrote: >> Right. I don't like the use of this attribute on labels at all, for >> the reasons you list here. I think it would be much cleaner to add a >> branch hint on the label in the asm goto, to contain this extension >> and to also to make it clear that it's not the label that is cold but >> the jump that is unlikely to be executed (i.e. cause and effect: the >> jump is unlikely and therefore the basic block is cold). > > As in the case where you have both an unlikely and likely jump to a > basic-block. But what I understand is that rth adds a way to mark > a basic-block as hot or cold, not a way to mark an edge as hot or cold > (that would be what the asm goto annotation would do). Both cases > are of course useful. I don't see why it is useful to be able to mark a basic block as hot or cold. This is something that the compiler can figure out for itself if you provide the branch hints (__builtin_expect is also a kind of branch hint). Marking basic blocks as likely or unlikely seems just redundant and confusing to me. A basic block being hot or cold is an effect of its incoming edges being unlikely-taken, not an inherent property of the basic block itself. Ciao! Steven
On Fri, Jul 27, 2012 at 11:40 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > On Fri, Jul 27, 2012 at 11:15 AM, Richard Guenther > <richard.guenther@gmail.com> wrote: >>> Right. I don't like the use of this attribute on labels at all, for >>> the reasons you list here. I think it would be much cleaner to add a >>> branch hint on the label in the asm goto, to contain this extension >>> and to also to make it clear that it's not the label that is cold but >>> the jump that is unlikely to be executed (i.e. cause and effect: the >>> jump is unlikely and therefore the basic block is cold). >> >> As in the case where you have both an unlikely and likely jump to a >> basic-block. But what I understand is that rth adds a way to mark >> a basic-block as hot or cold, not a way to mark an edge as hot or cold >> (that would be what the asm goto annotation would do). Both cases >> are of course useful. > > I don't see why it is useful to be able to mark a basic block as hot > or cold. This is something that the compiler can figure out for itself > if you provide the branch hints (__builtin_expect is also a kind of > branch hint). Marking basic blocks as likely or unlikely seems just > redundant and confusing to me. A basic block being hot or cold is an > effect of its incoming edges being unlikely-taken, not an inherent > property of the basic block itself. Well, I see it as a more finegrained way to force the compiler to optimize that block for size (which then leads to my question on basic-block merging and "propagating" that info). Richard. > Ciao! > Steven
Il 27/07/2012 11:40, Steven Bosscher ha scritto: >> > >> > As in the case where you have both an unlikely and likely jump to a >> > basic-block. But what I understand is that rth adds a way to mark >> > a basic-block as hot or cold, not a way to mark an edge as hot or cold >> > (that would be what the asm goto annotation would do). Both cases >> > are of course useful. > I don't see why it is useful to be able to mark a basic block as hot > or cold. This is something that the compiler can figure out for itself > if you provide the branch hints (__builtin_expect is also a kind of > branch hint). Marking basic blocks as likely or unlikely seems just > redundant and confusing to me. A basic block being hot or cold is an > effect of its incoming edges being unlikely-taken, not an inherent > property of the basic block itself. You could say the same of functions. Sometimes it's easier to mark the edges and sometimes it's easier to mark the targets. Paolo
On 07/27/2012 01:57 AM, Richard Guenther wrote: > On Thu, Jul 26, 2012 at 11:58 PM, Richard Henderson <rth@redhat.com> wrote: >> On 07/26/2012 02:41 PM, Richard Henderson wrote: >>> This is a patch... >> >> ... that I should have attached. Bah. > > Do we need to mark the labels so we preserve them? Consider > > goto foo; > > foo: > bar __attribute__((cold)): > ... > > so bar will be unused? We don't purge unused labels until rtl (at which point it becomes a deleted label note), and we only really need the label to survive until after the profile_estimate pass. After which all the significance of the label has been transferred into the edge frequency. The test case void g(void); void h(void); void f(int x, int y) { if (x) goto A; return; A: B: __attribute__((cold)) g(); return; } does in fact DTRT with the estimates. > What about BB merging if we end up with > > <BB 3>: > .. > fallthru > bar __attribute__((cold)): > ... > > should BB 3 inherit the coldness? I think we no longer disable > BB merging if the destination has user labels. The edge might be marked cold, but that should have no other effect. No more than if (__builtin_expect (test, 0)) when it turns out that we can prove that test is false. r~
On 07/27/2012 02:08 AM, Steven Bosscher wrote: > Right. I don't like the use of this attribute on labels at all, for > the reasons you list here. I think it would be much cleaner to add a > branch hint on the label in the asm goto, to contain this extension > and to also to make it clear that it's not the label that is cold but > the jump that is unlikely to be executed (i.e. cause and effect: the > jump is unlikely and therefore the basic block is cold). The label attribute is also usable for computed goto. Are you going to change the syntax for that one as well? r~
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index b72506b..a002541 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -6169,7 +6169,8 @@ static tree handle_hot_attribute (tree *node, tree name, tree ARG_UNUSED (args), int ARG_UNUSED (flags), bool *no_add_attrs) { - if (TREE_CODE (*node) == FUNCTION_DECL) + if (TREE_CODE (*node) == FUNCTION_DECL + || TREE_CODE (*node) == LABEL_DECL) { if (lookup_attribute ("cold", DECL_ATTRIBUTES (*node)) != NULL) { @@ -6188,6 +6189,7 @@ handle_hot_attribute (tree *node, tree name, tree ARG_UNUSED (args), return NULL_TREE; } + /* Handle a "cold" and attribute; arguments as in struct attribute_spec.handler. */ @@ -6195,7 +6197,8 @@ 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) + if (TREE_CODE (*node) == FUNCTION_DECL + || TREE_CODE (*node) == LABEL_DECL) { if (lookup_attribute ("hot", DECL_ATTRIBUTES (*node)) != NULL) { diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index c3faf09..5d851a7 100644 --- a/gcc/doc/extend.texi +++ b/gcc/doc/extend.texi @@ -3345,33 +3345,53 @@ than 2.96. @item hot @cindex @code{hot} function attribute -The @code{hot} attribute is used to inform the compiler that a function is a -hot spot of the compiled program. The function is optimized more aggressively -and on many target it is placed into special subsection of the text section so -all hot functions appears close together improving locality. +The @code{hot} attribute on a function is used to inform the compiler that +the function is a hot spot of the compiled program. The function is +optimized more aggressively and on many target it is placed into special +subsection of the text section so all hot functions appears close together +improving locality. When profile feedback is available, via @option{-fprofile-use}, hot functions are automatically detected and this attribute is ignored. -The @code{hot} attribute is not implemented in GCC versions earlier -than 4.3. +The @code{hot} attribute on functions is not implemented in GCC versions +earlier than 4.3. + +@cindex @code{hot} label attribute +The @code{hot} attribute on a label is used to inform the compiler that +path following the label are more likely than paths that are not so +annotated. This attribute is used in cases where @code{__builtin_expect} +cannot be used, for instance with computed goto or @code{asm goto}. + +The @code{hot} attribute on labels is not implemented in GCC versions +earlier than 4.8. @item cold @cindex @code{cold} function attribute -The @code{cold} attribute is used to inform the compiler that a function is -unlikely executed. The function is optimized for size rather than speed and on -many targets it is placed into special subsection of the text section so all -cold functions appears close together improving code locality of non-cold parts -of program. The paths leading to call of cold functions within code are marked -as unlikely by the branch prediction mechanism. It is thus useful to mark -functions used to handle unlikely conditions, such as @code{perror}, as cold to -improve optimization of hot functions that do call marked functions in rare -occasions. - -When profile feedback is available, via @option{-fprofile-use}, hot functions +The @code{cold} attribute on functions is used to inform the compiler that +the function is unlikely to be executed. The function is optimized for +size rather than speed and on many targets it is placed into special +subsection of the text section so all cold functions appears close together +improving code locality of non-cold parts of program. The paths leading +to call of cold functions within code are marked as unlikely by the branch +prediction mechanism. It is thus useful to mark functions used to handle +unlikely conditions, such as @code{perror}, as cold to improve optimization +of hot functions that do call marked functions in rare occasions. + +When profile feedback is available, via @option{-fprofile-use}, cold functions are automatically detected and this attribute is ignored. -The @code{cold} attribute is not implemented in GCC versions earlier than 4.3. +The @code{cold} attribute on functions is not implemented in GCC versions +earlier than 4.3. + +@cindex @code{cold} label attribute +The @code{cold} attribute on labels is used to inform the compiler that +the path following the label is unlikely to be executed. This attribute +is used in cases where @code{__builtin_expect} cannot be used, for instance +with computed goto or @code{asm goto}. + +The @code{cold} attribute on labels is not implemented in GCC versions +earlier than 4.8. @item regparm (@var{number}) @cindex @code{regparm} attribute diff --git a/gcc/predict.c b/gcc/predict.c index b690bdc..b8acdba 100644 --- a/gcc/predict.c +++ b/gcc/predict.c @@ -2059,6 +2059,29 @@ tree_estimate_probability_bb (basic_block bb) FOR_EACH_EDGE (e, ei, bb->succs) { + /* Predict edges to user labels with attributes. */ + if (e->dest != EXIT_BLOCK_PTR) + { + gimple_stmt_iterator gi; + for (gi = gsi_start_bb (e->dest); !gsi_end_p (gi); gsi_next (&gi)) + { + gimple stmt = gsi_stmt (gi); + tree decl; + + if (gimple_code (stmt) != GIMPLE_LABEL) + break; + decl = gimple_label_label (stmt); + if (DECL_ARTIFICIAL (decl)) + continue; + + /* Finally, we have a user-defined label. */ + if (lookup_attribute ("cold", DECL_ATTRIBUTES (decl))) + predict_edge_def (e, PRED_COLD_LABEL, NOT_TAKEN); + else if (lookup_attribute ("hot", DECL_ATTRIBUTES (decl))) + predict_edge_def (e, PRED_HOT_LABEL, TAKEN); + } + } + /* Predict early returns to be probable, as we've already taken care for error returns and other cases are often used for fast paths through function. diff --git a/gcc/predict.def b/gcc/predict.def index 591bb1c..7e77f14 100644 --- a/gcc/predict.def +++ b/gcc/predict.def @@ -126,3 +126,10 @@ DEF_PREDICTOR (PRED_LOOP_IV_COMPARE_GUESS, "guess loop iv compare", to set probability of branches that compares IV to loop bound variable. */ DEF_PREDICTOR (PRED_LOOP_IV_COMPARE, "loop iv compare", PROB_VERY_LIKELY, PRED_FLAG_FIRST_MATCH) + +/* Branches to hot labels are likely. */ +DEF_PREDICTOR (PRED_HOT_LABEL, "hot label", HITRATE (85), 0) + +/* Branches to cold labels are extremely unlikely. */ +DEF_PREDICTOR (PRED_COLD_LABEL, "cold label", PROB_VERY_LIKELY, + PRED_FLAG_FIRST_MATCH) diff --git a/gcc/testsuite/gcc.dg/attr-hotcold-1.c b/gcc/testsuite/gcc.dg/attr-hotcold-1.c new file mode 100644 index 0000000..f63a95c --- /dev/null +++ b/gcc/testsuite/gcc.dg/attr-hotcold-1.c @@ -0,0 +1,8 @@ +void f(void) +{ + goto A; + A: __attribute__((cold)) + goto B; + B: __attribute__((hot)) + return; +} diff --git a/gcc/testsuite/gcc.dg/tree-ssa/attr-hotcold-2.c b/gcc/testsuite/gcc.dg/tree-ssa/attr-hotcold-2.c new file mode 100644 index 0000000..84327fe --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/attr-hotcold-2.c @@ -0,0 +1,28 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-ipa-profile_estimate-details" } */ + +void g(void); +void h(void); +void f(int x, int y) +{ + if (x) goto A; + if (y) goto B; + return; + + A: __attribute__((cold)) + g(); + return; + + B: __attribute__((hot)) + h(); + return; +} + +/* { dg-final { scan-ipa-dump-times "block 4, loop depth 0, count 0, freq 1\[^0-9\]" 1 "profile_estimate" } } */ + +/* Note: we're attempting to match some number > 6000, i.e. > 60%. + The exact number ought to be tweekable without having to juggle + the testcase around too much. */ +/* { dg-final { scan-ipa-dump-times "block 5, loop depth 0, count 0, freq \[6-9\]\[0-9\]\[0-9\]\[0-9\]" 1 "profile_estimate" } } */ + +/* { dg-final { cleanup-tree-dump "profile_estimate" } } */
On 07/26/2012 02:41 PM, Richard Henderson wrote: > This is a patch... ... that I should have attached. Bah. r~ gcc/ * doc/extend.texi (attribute): Document hot/cold for labels. * predict.c (tree_estimate_probability_bb): Handle hot/cold attributes on user labels. * predict.def (PRED_HOT_LABEL, PRED_COLD_LABEL): New. gcc/c-family/ * c-common.c (handle_hot_attribute): Allow labels. (handle_cold_attribute): Likewise. gcc/testsuite/ * gcc.dg/attr-hotcold-1.c: New. * gcc.dg/tree-ssa/attr-hotcold-2.c: New.