On Thu, Aug 7, 2014 at 2:20 PM, Wei Mi <w...@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 <davi...@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 <w...@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 <w...@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)
>>>
>>>
>>>
>>>
>>>
>>>
