Patchwork Add hot/cold attributes for labels

login
register
mail settings
Submitter Richard Henderson
Date July 26, 2012, 9:58 p.m.
Message ID <5011BD69.4080503@redhat.com>
Download mbox | patch
Permalink /patch/173527/
State New
Headers show

Comments

Richard Henderson - July 26, 2012, 9:58 p.m.
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.
Richard Guenther - July 27, 2012, 8:57 a.m.
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.
Steven Bosscher - July 27, 2012, 9:08 a.m.
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
Richard Guenther - July 27, 2012, 9:15 a.m.
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
Steven Bosscher - July 27, 2012, 9:40 a.m.
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
Richard Guenther - July 27, 2012, 10:09 a.m.
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
Paolo Bonzini - July 27, 2012, 12:16 p.m.
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
Richard Henderson - July 27, 2012, 1:56 p.m.
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~
Richard Henderson - July 27, 2012, 1:57 p.m.
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~

Patch

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" } } */