[PATCH] D114400: [SampleFDO] Compute BFI if the sample loader changes BPI in FSAFDO

[Rong Xu via llvm-commits]

On Tue, Nov 23, 2021 at 3:43 PM Wenlei He via Phabricator <
reviews at reviews.llvm.org> wrote:

> wenlei added inline comments.
>
>
> ================
> Comment at: llvm/lib/CodeGen/MIRSampleProfile.cpp:318
> +  if (Changed)
> +    MBFI->calculate(MF, *MBFI->getMBPI(),
> *&getAnalysis<MachineLoopInfo>());
>
> ----------------
> hoy wrote:
> > wenlei wrote:
> > > 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.
> > >
> > > It feels to me that updating MBFI inside sample loader kind of breaks
> the componentization.
> > 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.
> > But it's a bit special here in that we are using the BFI right after
> here.
>
> What is the use you're referring to? I only the use of `MBFI->View` which
> is not a real use.
>
> I don't see it as a reason to introduce this tight coupling.


The original fix was to comment out AU.setPreservesAll in
getAnalysisUsage().
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).
As a matter or fact, we do use this option quite a bit.

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.


> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D114400/new/
>
> https://reviews.llvm.org/D114400
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20211123/438f46bb/attachment.html>