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

[Rong Xu via llvm-commits]

Thanks David for explaining this for me. Sorry for not making it clear in
the original email.

On Sat, Feb 6, 2021 at 2:53 PM Xinliang David Li <davidxl at google.com> wrote:

> 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)
>>
> Yes. I thought about the alternative way you mentioned. Currently there
are just two places to use the code, one for IR and one for CodeGen.  They
are instantiated to different types so There is not much reuse of the
object. So I chose to use a header-only version. I think it's good enough
for now.

>
>> 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/20210208/092e3529/attachment.html>