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

[Hongtao Yu via Phabricator via llvm-commits]

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