diff mbox

[GOOGLE,AUTOFDO] Assign different discriminators to calls with the same lineno

Message ID CA+4CFy6Ka-KdZ9dzth671mGBvkwX2hkaVDnZffMqJhOx41js1g@mail.gmail.com
State New
Headers show

Commit Message

Wei Mi Aug. 29, 2014, 8:59 p.m. UTC 
> On Fri, Aug 29, 2014 at 10:11 AM, Cary Coutant <ccoutant@google.com> wrote:
>>> To avoid the unused new discriminator value, I added a map
>>> "found_call_this_line" to track whether a call is the first call in a
>>> source line seen when assigning discriminators. For the first call in
>>> a source line, its discriminator is 0. For the following calls in the
>>> same source line, a new discriminator will be used everytime. The new
>>> patch is attached. Internal perf test and regression test are ok. Is
>>> it ok for google-4_9?
>>
>> This seems overly complex to me. I'd think all you need to do is add a
>> bit to locus_discrim_map (stealing a bit from discriminator ought to
>> be fine) that indicates whether the next call should increment the
>> discriminator or not. Something like this:
>>
>> increase_discriminator_for_locus (location_t locus, bool return_next)
>> {
>>   ...
>>   if (return_next || (*slot)->needs_increment)
>>     {
>>       (*slot)->discriminator++;
>>       (*slot)->needs_increment = false;
>>     }
>>   else
>>     (*slot)->needs_increment = true;
>>   return (*slot)->discriminator;
>> }
>>
>> -cary

Here is the new patch (attached). Regression test passes. Cary, is it ok?

Thanks,
Wei.
ChangeLog:

2014-08-29  Wei Mi  <wmi@google.com>

	* tree-cfg.c (struct locus_discrim_map): New field needs_increment.
	(next_discriminator_for_locus): Increase discriminator only when
	return_next or needs_increment are true.
	(assign_discriminator): Add an actual for next_discriminator_for_locus.
	(assign_discriminators): Assign different discriminators for calls
	belonging to the same source line.

Comments

Cary Coutant Aug. 29, 2014, 10:01 p.m. UTC | #1 
2014-08-29  Wei Mi  <wmi@google.com>

        * tree-cfg.c (struct locus_discrim_map): New field needs_increment.
        (next_discriminator_for_locus): Increase discriminator only when
        return_next or needs_increment are true.
        (assign_discriminator): Add an actual for next_discriminator_for_locus.
        (assign_discriminators): Assign different discriminators for calls
        belonging to the same source line.

OK for google/gcc-4_9 branch. Thanks!

-cary

On Fri, Aug 29, 2014 at 1:59 PM, Wei Mi <wmi@google.com> wrote:
>> On Fri, Aug 29, 2014 at 10:11 AM, Cary Coutant <ccoutant@google.com> wrote:
>>>> To avoid the unused new discriminator value, I added a map
>>>> "found_call_this_line" to track whether a call is the first call in a
>>>> source line seen when assigning discriminators. For the first call in
>>>> a source line, its discriminator is 0. For the following calls in the
>>>> same source line, a new discriminator will be used everytime. The new
>>>> patch is attached. Internal perf test and regression test are ok. Is
>>>> it ok for google-4_9?
>>>
>>> This seems overly complex to me. I'd think all you need to do is add a
>>> bit to locus_discrim_map (stealing a bit from discriminator ought to
>>> be fine) that indicates whether the next call should increment the
>>> discriminator or not. Something like this:
>>>
>>> increase_discriminator_for_locus (location_t locus, bool return_next)
>>> {
>>>   ...
>>>   if (return_next || (*slot)->needs_increment)
>>>     {
>>>       (*slot)->discriminator++;
>>>       (*slot)->needs_increment = false;
>>>     }
>>>   else
>>>     (*slot)->needs_increment = true;
>>>   return (*slot)->discriminator;
>>> }
>>>
>>> -cary
>
> Here is the new patch (attached). Regression test passes. Cary, is it ok?
>
> Thanks,
> Wei.
diff mbox

Patch

Index: tree-cfg.c
===================================================================
--- tree-cfg.c	(revision 213402)
+++ tree-cfg.c	(working copy)
@@ -112,7 +112,14 @@  static struct cfg_stats_d cfg_stats;
 struct locus_discrim_map
 {
   location_t locus;
-  int discriminator;
+  /* Different calls belonging to the same source line will be assigned
+     different discriminators. But we want to keep the discriminator of
+     the first call in the same source line to be 0, in order to reduce
+     the .debug_line section size. needs_increment is used for this
+     purpose. It is initialized as false and will be set to true after
+     the first call is seen.  */
+  bool needs_increment:1;
+  int discriminator:31;
 };
 
 /* Hashtable helpers.  */
@@ -914,10 +921,15 @@  make_edges (void)
 /* Find the next available discriminator value for LOCUS.  The
    discriminator distinguishes among several basic blocks that
    share a common locus, allowing for more accurate sample-based
-   profiling.  */
+   profiling. If RETURN_NEXT is true, return the next discriminator
+   anyway. If RETURN_NEXT is not true, we may not increase the
+   discriminator if locus_discrim_map::needs_increment is false,
+   which is used when the stmt is the first call stmt in current
+   source line. locus_discrim_map::needs_increment will be set to
+   true after the first call is seen.  */
 
 static int
-next_discriminator_for_locus (location_t locus)
+next_discriminator_for_locus (location_t locus, bool return_next)
 {
   struct locus_discrim_map item;
   struct locus_discrim_map **slot;
@@ -932,9 +944,13 @@  next_discriminator_for_locus (location_t
       *slot = XNEW (struct locus_discrim_map);
       gcc_assert (*slot);
       (*slot)->locus = locus;
+      (*slot)->needs_increment = false;
       (*slot)->discriminator = 0;
     }
-  (*slot)->discriminator++;
+  if (return_next || (*slot)->needs_increment)
+    (*slot)->discriminator++;
+  else
+    (*slot)->needs_increment = true;
   return (*slot)->discriminator;
 }
 
@@ -974,7 +990,7 @@  assign_discriminator (location_t locus,
   if (locus == UNKNOWN_LOCATION)
     return;
 
-  discriminator = next_discriminator_for_locus (locus);
+  discriminator = next_discriminator_for_locus (locus, true);
 
   for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
     {
@@ -1009,23 +1025,13 @@  assign_discriminators (void)
       for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
 	{
 	  gimple stmt = gsi_stmt (gsi);
-	  if (curr_locus == UNKNOWN_LOCATION)
-	    {
+          if (gimple_code (stmt) == GIMPLE_CALL)
+            {
 	      curr_locus = gimple_location (stmt);
-	    }
-	  else if (!same_line_p (curr_locus, gimple_location (stmt)))
-	    {
-	      curr_locus = gimple_location (stmt);
-	      curr_discr = 0;
-	    }
-	  else if (curr_discr != 0)
-	    {
+	      curr_discr = next_discriminator_for_locus (curr_locus, false);
 	      gimple_set_location (stmt, location_with_discriminator (
-		  gimple_location (stmt), curr_discr));
+		  curr_locus, curr_discr));
 	    }
-	  /* Allocate a new discriminator for CALL stmt.  */
-	  if (gimple_code (stmt) == GIMPLE_CALL)
-	    curr_discr = next_discriminator_for_locus (curr_locus);
 	}
 
       if (locus == UNKNOWN_LOCATION)