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

[Xinliang David Li <davidxl at google dot com>]

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