[PATCH] D95832: [SampleFDO][NFC] Refacrot SampleProfileLoad to reuse the code in MachineIR

[Xinliang David Li via llvm-commits]

no it is not noise -- the questions you raised are  very good :)

On Sat, Feb 6, 2021 at 2:49 PM David Blaikie via Phabricator <
reviews at reviews.llvm.org> wrote:

> dblaikie added a comment.
>
> In D95832#2547119 <https://reviews.llvm.org/D95832#2547119>, @davidxl
> wrote:
>
> > I think what Rong means is that
> >
> > 1. moving code to a header is for code sharing between IR and codeGenPass
> > 2. Wei's idea of avoiding the header move can lead to circular
> dependencies.
> >
> > In other words, doing 1) is not to achieve circular dependency avoidance,
> > but just using 2) as an alternative way is not working.
>
> I'm not quite following - if there's no .cc file where the implementation
> of the header could live, if we wanted to write some of it out of line,
> that seems like there is a circular dependency at the source/design level
> even if it doesn't result in linker errors. It sounds like (2) is showing
> that (1) has a hidden circular dependency.
>
> > That idea did not work: I place a new cpp file into
> llvm/lib/ProfileData. But that creates a circular dependency b/w
> ProfileData and CodeGen.
>
> Oh, I think I understand now - these are (going to be) templates, and so
> long as they remain templates that are implicitly instantiated, there's no
> dependency problem - but if we tried to explicitly instantiate the CodeGen
> version inside ProfileData, that would create a circular dependency. *nod*
>
> Yes, using out-of-class-but-still marked 'inline' definitions (once
> they're templates they don't even need the inline keyword) may help
> readability by making the class easier to read through without long
> function bodies interleaved. (the only other option would be to put the
> implementation in a whole separate file and have some place in each library
> that includes both interface and implementation headers and performs the
> explicit instantiation there (in the appropriate libraries) - but that's
> probably overkill unless the header is included in many different places in
> each library, such that the explicit instantiation could significantly
> reduce compile-time/object size, etc)
>
> Right right. -thanks for helping me understand. Sorry for the noise.
>
> The goal here is to have Transforms/IPO/
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D95832/new/
>
> https://reviews.llvm.org/D95832
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210206/4f56781c/attachment.html>