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

[Rong Xu via llvm-commits]

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.

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/b5b7f5c5/attachment.html>