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

[David Blaikie via Phabricator via llvm-commits]

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