<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 23, 2021 at 3:43 PM Wenlei He via Phabricator <<a href="mailto:reviews@reviews.llvm.org">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">wenlei added inline comments.<br>
<br>
<br>
================<br>
Comment at: llvm/lib/CodeGen/MIRSampleProfile.cpp:318<br>
+  if (Changed)<br>
+    MBFI->calculate(MF, *MBFI->getMBPI(), *&getAnalysis<MachineLoopInfo>());<br>
<br>
----------------<br>
hoy wrote:<br>
> wenlei wrote:<br>
> > When profile loader leads to change, we should probably recalculate BFI. But should BFI calculation be done outside of sample loader given BFI is a separate analysis? The IR BFI is done outside of sample loader. <br>
> > <br>
> > It feels to me that updating MBFI inside sample loader kind of breaks the componentization. <br>
> Normally you would mark this pass to not preserve BFI and rely on subsequent LazyBFI pass to update BFI automatically. But it's a bit special here in that we are using the BFI right after here.<br>
> But it's a bit special here in that we are using the BFI right after here. <br>
<br>
What is the use you're referring to? I only the use of `MBFI->View` which is not a real use. <br>
<br>
I don't see it as a reason to introduce this tight coupling.</blockquote><div><br></div><div>The original fix was to comment out AU.setPreservesAll in getAnalysisUsage().</div><div>I chose not to do that, like hongtao mentioned, because I want to display the correct BFI within this pass (for option -fs-viewbfi-after).</div><div>As a matter or fact, we do use this option quite a bit.</div><div><br></div><div>I think with updated BPI, we know for sure BFI needs to be recomputed. I don't feel that bad to update BFI within this pass.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Repository:<br>
  rG LLVM Github Monorepo<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D114400/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D114400/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D114400" rel="noreferrer" target="_blank">https://reviews.llvm.org/D114400</a><br>
<br>
</blockquote></div></div>