<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-family:times new roman,serif"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jun 18, 2020 at 10:44 AM Rong Xu <<a href="mailto:xur@google.com">xur@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">I think the right place is to change PGO Instrumentation. It's very easy and Both Gen and Use share the same code.<div><br></div><div>My previous patch was done exactly like that. I can dig out that patch.</div><div><br></div><div>I remember that I also have an option to force the entry node update atomically so that the entry counts are more reliable. </div></div></blockquote><div><br></div><div><div class="gmail_default" style="font-family:"times new roman",serif">If you can share your patch, that will be great!</div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jun 17, 2020 at 11:11 PM Hongtao Yu via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">hoyFB added a comment.<br>
<br>
In D81981#2099825 <<a href="https://reviews.llvm.org/D81981#2099825" rel="noreferrer" target="_blank">https://reviews.llvm.org/D81981#2099825</a>>, @wmi wrote:<br>
<br>
> In D81981#2099769 <<a href="https://reviews.llvm.org/D81981#2099769" rel="noreferrer" target="_blank">https://reviews.llvm.org/D81981#2099769</a>>, @davidxl wrote:<br>
><br>
> ><br>
><br>
><br>
> The index format needs to be backward compatible, so there needs to be some version specific handling there (can be removed later).<br>
><br>
> 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?<br>
><br>
> >> 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.<br>
> >> <br>
> >> 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.<br>
> > <br>
> > The current PGO instrumentation is based on MST.<br>
><br>
> 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.<br>
><br>
> > 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 ?<br>
><br>
> 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.<br>
<br>
<br>
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.<br>
<br>
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.<br>
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D81981/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D81981/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D81981" rel="noreferrer" target="_blank">https://reviews.llvm.org/D81981</a><br>
<br>
<br>
<br>
</blockquote></div>
</blockquote></div></div>