[PATCH] D81981: [PGO] Supplement PGO profile with Sample profile

[Wei Mi via llvm-commits]

On Thu, Jun 18, 2020 at 10:44 AM Rong Xu <xur at google.com> wrote:

> I think the right place is to change PGO Instrumentation. It's very easy
> and Both Gen and Use share the same code.
>
> My previous patch was done exactly like that. I can dig out that patch.
>
> I remember that I also have an option to force the entry node update
> atomically so that the entry counts are more reliable.
>

If you can share your patch, that will be great!



>
> On Wed, Jun 17, 2020 at 11:11 PM Hongtao Yu via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> hoyFB added a comment.
>>
>> In D81981#2099825 <https://reviews.llvm.org/D81981#2099825>, @wmi wrote:
>>
>> > In D81981#2099769 <https://reviews.llvm.org/D81981#2099769>, @davidxl
>> wrote:
>> >
>> > >
>> >
>> >
>> > The index format needs to be backward compatible, so there needs to be
>> some version specific handling there (can be removed later).
>> >
>> > I don't understand this part. Could you elaborate it -- why index
>> format is different from raw format in backward compatiblity, and what is
>> the version specific handling?
>> >
>> > >> For function entry bb which have multiple successors, the existing
>> algorithm in FuncPGOInstrumentation<Edge, BBInfo>::getInstrBB will insert
>> the counter in all its successors. My current implementation simply adds a
>> counter in entry block so in that case, it introduces redundent counter.
>> > >>
>> > >> I can improve it by selecting a successor to not insert counter for
>> it since it can be inferred from the counters surrounding it. With that
>> implemented, I expect the profile size will be unchanged.
>> > >
>> > > The current PGO instrumentation is based on MST.
>> >
>> > Yes, it is based on two parts, selecting MST is one part and selecting
>> src/dst node of each non-MST edge to instrument is another part.
>> >
>> > > Changing the instrumentation may require changes in how the counts of
>> non-MST-edges are calculated (in `PGOUseFunc::setInstrumentedCounts`). So
>> maybe adjust the MST to remove the sibling edge ?
>> >
>> > The change to add entry BB as an instrumented BB is in function
>> getInstrumentBBs which is shared by profile-gen and profile-use, so it will
>> be consistent between profile-gen and profile-use. About adjusting MST to
>> remove sibling edge, I feel it is inconsistent with current goal of MST
>> selection. The goal of selecting MST is to avoid instrumenting the most
>> frequent edges so we can minimize the cost. Removing a successor edge of
>> the entry block is a different goal. Mixing these two goals will make
>> things complicated. I feel it is simpler to add the change in the second
>> part -- selecting between src and dst which node to instrument.
>>
>>
>> I see. Yes, it's reasonable to avoid changing MST edges, instead, to
>> change which block to instrument for a given edge. I was thinking special
>> logic may be needed for the edge count calculation as well, since it's
>> related to where the instrumentation happens. This should work if we change
>> both places.
>>
>> Since only non-MST edges are instrumented, I was wondering alternatively
>> we can remove an edge related to the entry block from MST to force the
>> entry instrumented. I think removing the fake entry edge as David suggested
>> is better than removing an outgoing sibling edge from the entry. Removing
>> the fake entry edge from MST will result in one of the outgoing sibling
>> edges added to MST, which in turn will cause the corresponding successor of
>> the entry not instrumented.
>>
>>
>> Repository:
>>   rL LLVM
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D81981/new/
>>
>> https://reviews.llvm.org/D81981
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200618/bc695a15/attachment.html>