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

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

[Wei Mi]

(Sorry if you received the mail twice because it was not plain text at
first and was rejected by @sourceware.org)

We saw bb like this in the IR dump after pass_build_cfg:

  <bb 21>:
  [1.cc : 205:45] D.332088 = table->_vptr.Table;
  [1.cc : 205:45] D.332134 = D.332088 + 104;
  [1.cc : 205:45] D.332135 = [1.cc : 205] *D.332134;
  [1.cc : 205:45] D.332092 = [1.cc : 205] &this->cp_stream_;
  [1.cc : 205:46] OBJ_TYPE_REF(D.332135;(struct Table)table->13)
(table, cp_arg, D.332092);  // indirect call
  [1.cc : 179:64] Reader::~Reader (&version);
  [1.cc : 205:46] Switcher::~Switcher (&tcswr);

The indirect call above has the same source lineno with
"Switcher::~Switcher (&tcswr);", but they have no discriminator so
they cannot be discriminated in autofdo. This causes the problem that
autofdo mistakenly regards "Switcher::~Switcher (&tcswr);" as a target
of the indirect call above, and makes a wrong promotion.

The existing code has the logic to assign different discriminators to
calls with the same lineno, but it only works when the lineno in a bb
is monotonical. In this case, there is another stmt with lineno 179
between the indirect call and "Switcher::~Switcher (&tcswr);" (both
with lineno 205), so existing code will not assign different
discriminators for them.

The patch is to assign discriminators for calls with the same lineno anyway.

regression test is going. internal perf test for autofdo shows a
little improvement. Ok for google-4_9 if regression pass?

Thanks,
Wei.

ChangeLog:

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

        * tree-cfg.c (increase_discriminator_for_locus): It was
        next_discriminator_for_locus. Add a param "return_next".
        (next_discriminator_for_locus): Renamed.
        (assign_discriminator): Use the renamed func.
        (assign_discriminators): Assign different discriminators
        for calls with the same lineno.


Index: tree-cfg.c
===================================================================
--- tree-cfg.c  (revision 213402)
+++ tree-cfg.c  (working copy)
@@ -914,10 +914,12 @@ 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 discriminator
+   value after the increase, else return the discriminator value
+   before the increase.  */

 static int
-next_discriminator_for_locus (location_t locus)
+increase_discriminator_for_locus (location_t locus, bool return_next)
 {
   struct locus_discrim_map item;
   struct locus_discrim_map **slot;
@@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t
       (*slot)->locus = locus;
       (*slot)->discriminator = 0;
     }
+
   (*slot)->discriminator++;
-  return (*slot)->discriminator;
+  return return_next ? (*slot)->discriminator
+                    : (*slot)->discriminator - 1;
 }

 /* Return TRUE if LOCUS1 and LOCUS2 refer to the same source line.  */
@@ -974,7 +978,7 @@ assign_discriminator (location_t locus,
   if (locus == UNKNOWN_LOCATION)
     return;

-  discriminator = next_discriminator_for_locus (locus);
+  discriminator = increase_discriminator_for_locus (locus, true);

   for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
     {
@@ -1009,23 +1013,16 @@ 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)
-           {
-             curr_locus = gimple_location (stmt);
-           }
-         else if (!same_line_p (curr_locus, gimple_location (stmt)))
+         if (gimple_code (stmt) == GIMPLE_CALL)
            {
              curr_locus = gimple_location (stmt);
-             curr_discr = 0;
-           }
-         else if (curr_discr != 0)
-           {
-             gimple_set_location (stmt, location_with_discriminator (
-                 gimple_location (stmt), curr_discr));
+             /* return the current discriminator first, then increase the
+                discriminator for next call.  */
+             curr_discr = increase_discriminator_for_locus (curr_locus, false);
+             if (curr_discr != 0)
+               gimple_set_location (stmt, location_with_discriminator (
+                   gimple_location (stmt), 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)

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

[Xinliang David Li]

Is this

[1.cc : 179:64] Reader::~Reader (&version);

from an inline instance?

David

On Wed, Aug 6, 2014 at 10:18 AM, Wei Mi <wmi@google.com> wrote:
> We saw bb like this in the IR dump after pass_build_cfg:
>
>   <bb 21>:
>   [1.cc : 205:45] D.332088 = table->_vptr.Table;
>   [1.cc : 205:45] D.332134 = D.332088 + 104;
>   [1.cc : 205:45] D.332135 = [1.cc : 205] *D.332134;
>   [1.cc : 205:45] D.332092 = [1.cc : 205] &this->cp_stream_;
>   [1.cc : 205:46] OBJ_TYPE_REF(D.332135;(struct Table)table->13) (table,
> cp_arg, D.332092);  // indirect call
>   [1.cc : 179:64] Reader::~Reader (&version);
>   [1.cc : 205:46] Switcher::~Switcher (&tcswr);
>
> The indirect call above has the same source lineno with "Switcher::~Switcher
> (&tcswr);", but they have no discriminator so they cannot be discriminated
> in autofdo. This causes the problem that autofdo mistakenly regards
> "Switcher::~Switcher (&tcswr);" as a target of the indirect call above, and
> makes a wrong promotion.
>
> The existing code has the logic to assign different discriminators to calls
> with the same lineno, but it only works when the lineno in a bb is
> monotonical. In this case, there is another stmt with lineno 179 between the
> indirect call and "Switcher::~Switcher (&tcswr);" (both with lineno 205), so
> existing code will not assign different discriminators for them.
>
> The patch is to assign discriminators for calls with the same lineno anyway.
>
> regression test is going. internal perf test for autofdo shows a little
> improvement. Ok for google-4_9 if regression pass?
>
> Thanks,
> Wei.
>
> ChangeLog:
>
> 2014-08-06  Wei Mi  <wmi@google.com>
>
>         * tree-cfg.c (increase_discriminator_for_locus): It was
>         next_discriminator_for_locus. Add a param "return_next".
>         (next_discriminator_for_locus): Renamed.
>         (assign_discriminator): Use the renamed func.
>         (assign_discriminators): Assign different discriminators
>         for calls with the same lineno.
>
>
> Index: tree-cfg.c
> ===================================================================
> --- tree-cfg.c  (revision 213402)
> +++ tree-cfg.c  (working copy)
> @@ -914,10 +914,12 @@ 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 discriminator
> +   value after the increase, else return the discriminator value
> +   before the increase.  */
>
>  static int
> -next_discriminator_for_locus (location_t locus)
> +increase_discriminator_for_locus (location_t locus, bool return_next)
>  {
>    struct locus_discrim_map item;
>    struct locus_discrim_map **slot;
> @@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t
>        (*slot)->locus = locus;
>        (*slot)->discriminator = 0;
>      }
> +
>    (*slot)->discriminator++;
> -  return (*slot)->discriminator;
> +  return return_next ? (*slot)->discriminator
> +                    : (*slot)->discriminator - 1;
>  }
>
>  /* Return TRUE if LOCUS1 and LOCUS2 refer to the same source line.  */
> @@ -974,7 +978,7 @@ assign_discriminator (location_t locus,
>    if (locus == UNKNOWN_LOCATION)
>      return;
>
> -  discriminator = next_discriminator_for_locus (locus);
> +  discriminator = increase_discriminator_for_locus (locus, true);
>
>    for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>      {
> @@ -1009,23 +1013,16 @@ 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)
> -           {
> -             curr_locus = gimple_location (stmt);
> -           }
> -         else if (!same_line_p (curr_locus, gimple_location (stmt)))
> +         if (gimple_code (stmt) == GIMPLE_CALL)
>             {
>               curr_locus = gimple_location (stmt);
> -             curr_discr = 0;
> -           }
> -         else if (curr_discr != 0)
> -           {
> -             gimple_set_location (stmt, location_with_discriminator (
> -                 gimple_location (stmt), curr_discr));
> +             /* return the current discriminator first, then increase the
> +                discriminator for next call.  */
> +             curr_discr = increase_discriminator_for_locus (curr_locus,
> false);
> +             if (curr_discr != 0)
> +               gimple_set_location (stmt, location_with_discriminator (
> +                   gimple_location (stmt), 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)
>
>
>
>
>
>

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

[Wei Mi]

No, it is not. This IR is dumped before early inline -- just after
pass_build_cfg. The line number of the deconstructor is marked
according to where its constructor is located, instead of where it is
inserted. This is also problematic.

Wei.

On Thu, Aug 7, 2014 at 12:11 PM, Xinliang David Li <davidxl@google.com> wrote:
> Is this
>
> [1.cc : 179:64] Reader::~Reader (&version);
>
> from an inline instance?
>
> David
>
> On Wed, Aug 6, 2014 at 10:18 AM, Wei Mi <wmi@google.com> wrote:
>> We saw bb like this in the IR dump after pass_build_cfg:
>>
>>   <bb 21>:
>>   [1.cc : 205:45] D.332088 = table->_vptr.Table;
>>   [1.cc : 205:45] D.332134 = D.332088 + 104;
>>   [1.cc : 205:45] D.332135 = [1.cc : 205] *D.332134;
>>   [1.cc : 205:45] D.332092 = [1.cc : 205] &this->cp_stream_;
>>   [1.cc : 205:46] OBJ_TYPE_REF(D.332135;(struct Table)table->13) (table,
>> cp_arg, D.332092);  // indirect call
>>   [1.cc : 179:64] Reader::~Reader (&version);
>>   [1.cc : 205:46] Switcher::~Switcher (&tcswr);
>>
>> The indirect call above has the same source lineno with "Switcher::~Switcher
>> (&tcswr);", but they have no discriminator so they cannot be discriminated
>> in autofdo. This causes the problem that autofdo mistakenly regards
>> "Switcher::~Switcher (&tcswr);" as a target of the indirect call above, and
>> makes a wrong promotion.
>>
>> The existing code has the logic to assign different discriminators to calls
>> with the same lineno, but it only works when the lineno in a bb is
>> monotonical. In this case, there is another stmt with lineno 179 between the
>> indirect call and "Switcher::~Switcher (&tcswr);" (both with lineno 205), so
>> existing code will not assign different discriminators for them.
>>
>> The patch is to assign discriminators for calls with the same lineno anyway.
>>
>> regression test is going. internal perf test for autofdo shows a little
>> improvement. Ok for google-4_9 if regression pass?
>>
>> Thanks,
>> Wei.
>>
>> ChangeLog:
>>
>> 2014-08-06  Wei Mi  <wmi@google.com>
>>
>>         * tree-cfg.c (increase_discriminator_for_locus): It was
>>         next_discriminator_for_locus. Add a param "return_next".
>>         (next_discriminator_for_locus): Renamed.
>>         (assign_discriminator): Use the renamed func.
>>         (assign_discriminators): Assign different discriminators
>>         for calls with the same lineno.
>>
>>
>> Index: tree-cfg.c
>> ===================================================================
>> --- tree-cfg.c  (revision 213402)
>> +++ tree-cfg.c  (working copy)
>> @@ -914,10 +914,12 @@ 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 discriminator
>> +   value after the increase, else return the discriminator value
>> +   before the increase.  */
>>
>>  static int
>> -next_discriminator_for_locus (location_t locus)
>> +increase_discriminator_for_locus (location_t locus, bool return_next)
>>  {
>>    struct locus_discrim_map item;
>>    struct locus_discrim_map **slot;
>> @@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t
>>        (*slot)->locus = locus;
>>        (*slot)->discriminator = 0;
>>      }
>> +
>>    (*slot)->discriminator++;
>> -  return (*slot)->discriminator;
>> +  return return_next ? (*slot)->discriminator
>> +                    : (*slot)->discriminator - 1;
>>  }
>>
>>  /* Return TRUE if LOCUS1 and LOCUS2 refer to the same source line.  */
>> @@ -974,7 +978,7 @@ assign_discriminator (location_t locus,
>>    if (locus == UNKNOWN_LOCATION)
>>      return;
>>
>> -  discriminator = next_discriminator_for_locus (locus);
>> +  discriminator = increase_discriminator_for_locus (locus, true);
>>
>>    for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>>      {
>> @@ -1009,23 +1013,16 @@ 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)
>> -           {
>> -             curr_locus = gimple_location (stmt);
>> -           }
>> -         else if (!same_line_p (curr_locus, gimple_location (stmt)))
>> +         if (gimple_code (stmt) == GIMPLE_CALL)
>>             {
>>               curr_locus = gimple_location (stmt);
>> -             curr_discr = 0;
>> -           }
>> -         else if (curr_discr != 0)
>> -           {
>> -             gimple_set_location (stmt, location_with_discriminator (
>> -                 gimple_location (stmt), curr_discr));
>> +             /* return the current discriminator first, then increase the
>> +                discriminator for next call.  */
>> +             curr_discr = increase_discriminator_for_locus (curr_locus,
>> false);
>> +             if (curr_discr != 0)
>> +               gimple_set_location (stmt, location_with_discriminator (
>> +                   gimple_location (stmt), 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)
>>
>>
>>
>>
>>
>>

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

[Xinliang David Li]

On Thu, Aug 7, 2014 at 2:20 PM, Wei Mi <wmi@google.com> wrote:
> No, it is not. This IR is dumped before early inline -- just after
> pass_build_cfg. The line number of the deconstructor is marked
> according to where its constructor is located,

The definition location or the invocation location?

David

> instead of where it is
> inserted. This is also problematic.
>
> Wei.
>
> On Thu, Aug 7, 2014 at 12:11 PM, Xinliang David Li <davidxl@google.com> wrote:
>> Is this
>>
>> [1.cc : 179:64] Reader::~Reader (&version);
>>
>> from an inline instance?
>>
>> David
>>
>> On Wed, Aug 6, 2014 at 10:18 AM, Wei Mi <wmi@google.com> wrote:
>>> We saw bb like this in the IR dump after pass_build_cfg:
>>>
>>>   <bb 21>:
>>>   [1.cc : 205:45] D.332088 = table->_vptr.Table;
>>>   [1.cc : 205:45] D.332134 = D.332088 + 104;
>>>   [1.cc : 205:45] D.332135 = [1.cc : 205] *D.332134;
>>>   [1.cc : 205:45] D.332092 = [1.cc : 205] &this->cp_stream_;
>>>   [1.cc : 205:46] OBJ_TYPE_REF(D.332135;(struct Table)table->13) (table,
>>> cp_arg, D.332092);  // indirect call
>>>   [1.cc : 179:64] Reader::~Reader (&version);
>>>   [1.cc : 205:46] Switcher::~Switcher (&tcswr);
>>>
>>> The indirect call above has the same source lineno with "Switcher::~Switcher
>>> (&tcswr);", but they have no discriminator so they cannot be discriminated
>>> in autofdo. This causes the problem that autofdo mistakenly regards
>>> "Switcher::~Switcher (&tcswr);" as a target of the indirect call above, and
>>> makes a wrong promotion.
>>>
>>> The existing code has the logic to assign different discriminators to calls
>>> with the same lineno, but it only works when the lineno in a bb is
>>> monotonical. In this case, there is another stmt with lineno 179 between the
>>> indirect call and "Switcher::~Switcher (&tcswr);" (both with lineno 205), so
>>> existing code will not assign different discriminators for them.
>>>
>>> The patch is to assign discriminators for calls with the same lineno anyway.
>>>
>>> regression test is going. internal perf test for autofdo shows a little
>>> improvement. Ok for google-4_9 if regression pass?
>>>
>>> Thanks,
>>> Wei.
>>>
>>> ChangeLog:
>>>
>>> 2014-08-06  Wei Mi  <wmi@google.com>
>>>
>>>         * tree-cfg.c (increase_discriminator_for_locus): It was
>>>         next_discriminator_for_locus. Add a param "return_next".
>>>         (next_discriminator_for_locus): Renamed.
>>>         (assign_discriminator): Use the renamed func.
>>>         (assign_discriminators): Assign different discriminators
>>>         for calls with the same lineno.
>>>
>>>
>>> Index: tree-cfg.c
>>> ===================================================================
>>> --- tree-cfg.c  (revision 213402)
>>> +++ tree-cfg.c  (working copy)
>>> @@ -914,10 +914,12 @@ 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 discriminator
>>> +   value after the increase, else return the discriminator value
>>> +   before the increase.  */
>>>
>>>  static int
>>> -next_discriminator_for_locus (location_t locus)
>>> +increase_discriminator_for_locus (location_t locus, bool return_next)
>>>  {
>>>    struct locus_discrim_map item;
>>>    struct locus_discrim_map **slot;
>>> @@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t
>>>        (*slot)->locus = locus;
>>>        (*slot)->discriminator = 0;
>>>      }
>>> +
>>>    (*slot)->discriminator++;
>>> -  return (*slot)->discriminator;
>>> +  return return_next ? (*slot)->discriminator
>>> +                    : (*slot)->discriminator - 1;
>>>  }
>>>
>>>  /* Return TRUE if LOCUS1 and LOCUS2 refer to the same source line.  */
>>> @@ -974,7 +978,7 @@ assign_discriminator (location_t locus,
>>>    if (locus == UNKNOWN_LOCATION)
>>>      return;
>>>
>>> -  discriminator = next_discriminator_for_locus (locus);
>>> +  discriminator = increase_discriminator_for_locus (locus, true);
>>>
>>>    for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>>>      {
>>> @@ -1009,23 +1013,16 @@ 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)
>>> -           {
>>> -             curr_locus = gimple_location (stmt);
>>> -           }
>>> -         else if (!same_line_p (curr_locus, gimple_location (stmt)))
>>> +         if (gimple_code (stmt) == GIMPLE_CALL)
>>>             {
>>>               curr_locus = gimple_location (stmt);
>>> -             curr_discr = 0;
>>> -           }
>>> -         else if (curr_discr != 0)
>>> -           {
>>> -             gimple_set_location (stmt, location_with_discriminator (
>>> -                 gimple_location (stmt), curr_discr));
>>> +             /* return the current discriminator first, then increase the
>>> +                discriminator for next call.  */
>>> +             curr_discr = increase_discriminator_for_locus (curr_locus,
>>> false);
>>> +             if (curr_discr != 0)
>>> +               gimple_set_location (stmt, location_with_discriminator (
>>> +                   gimple_location (stmt), 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)
>>>
>>>
>>>
>>>
>>>
>>>

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

[Wei Mi]

On Thu, Aug 7, 2014 at 2:40 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Thu, Aug 7, 2014 at 2:20 PM, Wei Mi <wmi@google.com> wrote:
>> No, it is not. This IR is dumped before early inline -- just after
>> pass_build_cfg. The line number of the deconstructor is marked
>> according to where its constructor is located,
>
> The definition location or the invocation location?
>
> David
>

The definition location and the invocation location are the same
source line for the case.

Wei.

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

[Cary Coutant]

>  static int
> -next_discriminator_for_locus (location_t locus)
> +increase_discriminator_for_locus (location_t locus, bool return_next)
>  {
>    struct locus_discrim_map item;
>    struct locus_discrim_map **slot;
> @@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t
>        (*slot)->locus = locus;
>        (*slot)->discriminator = 0;
>      }
> +
>    (*slot)->discriminator++;
> -  return (*slot)->discriminator;
> +  return return_next ? (*slot)->discriminator
> +                    : (*slot)->discriminator - 1;
>  }

Won't this have the effect of sometimes incrementing the next
available discriminator without actually using the new value? That is,
if you call it once with return_next == false, and then with
return_next == true.

-cary

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

[Wei Mi]

Yes, that is intentional. It is to avoid assiging a discriminator for
the first call in the group of calls with the same source lineno.
Starting from the second call in the group, it will get a different
discriminator with previous call in the same group.

Thanks,
Wei.

On Thu, Aug 7, 2014 at 12:17 PM, Cary Coutant <ccoutant@google.com> wrote:
>>  static int
>> -next_discriminator_for_locus (location_t locus)
>> +increase_discriminator_for_locus (location_t locus, bool return_next)
>>  {
>>    struct locus_discrim_map item;
>>    struct locus_discrim_map **slot;
>> @@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t
>>        (*slot)->locus = locus;
>>        (*slot)->discriminator = 0;
>>      }
>> +
>>    (*slot)->discriminator++;
>> -  return (*slot)->discriminator;
>> +  return return_next ? (*slot)->discriminator
>> +                    : (*slot)->discriminator - 1;
>>  }
>
> Won't this have the effect of sometimes incrementing the next
> available discriminator without actually using the new value? That is,
> if you call it once with return_next == false, and then with
> return_next == true.
>
> -cary

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

[Wei Mi]

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

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?

Thanks,
Wei.



On Thu, Aug 7, 2014 at 2:10 PM, Wei Mi <wmi@google.com> wrote:
> Yes, that is intentional. It is to avoid assiging a discriminator for
> the first call in the group of calls with the same source lineno.
> Starting from the second call in the group, it will get a different
> discriminator with previous call in the same group.
>
> Thanks,
> Wei.
>
> On Thu, Aug 7, 2014 at 12:17 PM, Cary Coutant <ccoutant@google.com> wrote:
>>>  static int
>>> -next_discriminator_for_locus (location_t locus)
>>> +increase_discriminator_for_locus (location_t locus, bool return_next)
>>>  {
>>>    struct locus_discrim_map item;
>>>    struct locus_discrim_map **slot;
>>> @@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t
>>>        (*slot)->locus = locus;
>>>        (*slot)->discriminator = 0;
>>>      }
>>> +
>>>    (*slot)->discriminator++;
>>> -  return (*slot)->discriminator;
>>> +  return return_next ? (*slot)->discriminator
>>> +                    : (*slot)->discriminator - 1;
>>>  }
>>
>> Won't this have the effect of sometimes incrementing the next
>> available discriminator without actually using the new value? That is,
>> if you call it once with return_next == false, and then with
>> return_next == true.
>>
>> -cary

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 2566 bytes --]

ChangeLog:

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

	* tree-cfg.c (assign_discriminators): Assign different discriminators
	for calls belonging to the same source line.


Index: tree-cfg.c
===================================================================
--- tree-cfg.c	(revision 213402)
+++ tree-cfg.c	(working copy)
@@ -992,7 +992,13 @@ static void
 assign_discriminators (void)
 {
   basic_block bb;
+  /* If there is a location saved in the hash_table, it means that we
+     already found a call in the source line before. For the calls which
+     are not the first call found in the same source line, we don't assign
+     new discriminator for it, so that .debug_line section will be smaller.  */
+  hash_table <locus_discrim_hasher> found_call_this_line;
 
+  found_call_this_line.create (13);
   FOR_EACH_BB_FN (bb, cfun)
     {
       edge e;
@@ -1009,23 +1015,31 @@ 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)
-	    {
-	      curr_locus = gimple_location (stmt);
-	    }
-	  else if (!same_line_p (curr_locus, gimple_location (stmt)))
+	  if (gimple_code (stmt) == GIMPLE_CALL)
 	    {
+	      struct locus_discrim_map item;
+	      struct locus_discrim_map **slot;
+
 	      curr_locus = gimple_location (stmt);
-	      curr_discr = 0;
-	    }
-	  else if (curr_discr != 0)
-	    {
-	      gimple_set_location (stmt, location_with_discriminator (
-		  gimple_location (stmt), curr_discr));
+	      item.locus = curr_locus;
+	      item.discriminator = 0;
+	      slot = found_call_this_line.find_slot (&item, INSERT);
+	      /* If the current call is not the first call seen in curr_locus,
+		 assign the next discriminator to it, else keep its discriminator
+		 unchanged.  */
+	      if (*slot != HTAB_EMPTY_ENTRY)
+		{
+		  curr_discr = next_discriminator_for_locus (curr_locus);
+		  gimple_set_location (stmt, location_with_discriminator (
+				gimple_location (stmt), curr_discr));
+		}
+	      else
+		{
+		  *slot = XNEW (struct locus_discrim_map);
+		  (*slot)->locus = curr_locus;
+		  (*slot)->discriminator = 0;
+		}
 	    }
-	  /* 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)
@@ -1047,6 +1061,7 @@ assign_discriminators (void)
 	    }
 	}
     }
+  found_call_this_line.dispose ();
 }
 
 /* Create the edges for a GIMPLE_COND starting at block BB.  */

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

[Wei Mi]

Hi Cary,

Is the new patch ok for google-4_9?

Thanks,
Wei.


On Sun, Aug 24, 2014 at 8:53 PM, Wei Mi <wmi@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?
>
> Thanks,
> Wei.
>
>
>
> On Thu, Aug 7, 2014 at 2:10 PM, Wei Mi <wmi@google.com> wrote:
>> Yes, that is intentional. It is to avoid assiging a discriminator for
>> the first call in the group of calls with the same source lineno.
>> Starting from the second call in the group, it will get a different
>> discriminator with previous call in the same group.
>>
>> Thanks,
>> Wei.
>>
>> On Thu, Aug 7, 2014 at 12:17 PM, Cary Coutant <ccoutant@google.com> wrote:
>>>>  static int
>>>> -next_discriminator_for_locus (location_t locus)
>>>> +increase_discriminator_for_locus (location_t locus, bool return_next)
>>>>  {
>>>>    struct locus_discrim_map item;
>>>>    struct locus_discrim_map **slot;
>>>> @@ -934,8 +936,10 @@ next_discriminator_for_locus (location_t
>>>>        (*slot)->locus = locus;
>>>>        (*slot)->discriminator = 0;
>>>>      }
>>>> +
>>>>    (*slot)->discriminator++;
>>>> -  return (*slot)->discriminator;
>>>> +  return return_next ? (*slot)->discriminator
>>>> +                    : (*slot)->discriminator - 1;
>>>>  }
>>>
>>> Won't this have the effect of sometimes incrementing the next
>>> available discriminator without actually using the new value? That is,
>>> if you call it once with return_next == false, and then with
>>> return_next == true.
>>>
>>> -cary

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

[Cary Coutant]

> 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

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

[Wei Mi]

Thanks, that is ellegant. Will paste a new patch in this way soon.

Wei.

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

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

[Wei Mi]

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

> 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.

[-- Attachment #2: patch2.txt --]
[-- Type: text/plain, Size: 3682 bytes --]

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.

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)

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

[Cary Coutant]

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.