[PATCH] D29203: Change debug-info-for-profiling from a TargetOption to a function attribute.

[Mehdi Amini via llvm-commits]

> On Jan 27, 2017, at 11:29 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Fri, Jan 27, 2017 at 11:15 AM Dehao Chen via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
> danielcdh added a comment.
> 
> After a lengthy discussion with @dblaikie (Thanks David for the input), let me add some more context to this patch.
> 
> - What is the impact of adding -fdebug-info-for-profiling
>   - It will not affect the generated code
>   - It will increase the debug info size in -gmlt by ~17%
>   - It will not increase the debug info size in -g binary
> 
> ^ I take it that's true for the initial implementation, but not true shortly after (once discriminators and the newer discriminator feature (duplication factor, etc) are triggered off this flag as well), right? (but the relative impact on -g binaries, even under -gsplit-dwarf, will probably be pretty small in the grand scheme of things (there's still a lot of stuff to clean up in 
>  
>   - The compile time change should be negligible
> 
> - What we use a binary built with out -fdebug-info-for-profiling to collect profile
>   - The profile will be inaccurate and will yield sub-optimal performance when used to drive SamplePGO
> 
> - What is the motivating example that you want to compile some sources with -fdebug-info-for-profiling and some without
>   - If the debug info size is a concern for -gmlt binary, then one would want to only enable this flag for sources that is performance-critical
> 
> To put my 2c in to this & descirbe some of the higher level design decisions going on here:
> 
> * Generally it's seemed like LTO is made to behave like non-LTO where possible (eg: put attributes on functions or the DICompileUnit so that the effects are respected in the same functions with and without LTO).
> 
> * in cases where that isn't practical with the current implementation (eg: we emit a single line table, so we can't respect things that change the line table format on a per-function basis - the design could be changed to emit more than one line table, but the benefit doesn't seem worthwhile right now) a module flag is used and when modules are merged some rule for resolving the conflict (taking, say, the highest DWARF version) is chosen and possibly warnings are emitted
> 
> A deeper philosophical point arises as well: there are cases where "LTO behaves like non-LTO" can be supported but maybe /shouldn't/ be. The mental model I have for this situation is something like: If this situation represents a latent bug that could not be diagnosed in a traditional build (such as an ODR violation) then it's acceptable to break or change the behavior of LTO compared to non-LTO to address or depend on these conditions that should be satisfied but sometimes are not. (preferably with warnings, but sometimes not - no effort is made to diagnose debug info type definitions that differ/violate the ODR, one is chosen and the others discarded, for example)
> 
> So one of the questions here is whether -fdebug-info-for-profiling fits into this "any case of mismatches is really a user mistake that should be addressed, not a case that LLVM needs to respect".
> 
> 
> After that question is answered - if it's decided that it is best to respect the non-LTO behavior in LTO, then there's a question of how to implement this to respect the perf-function or per-CU behavior.
> 
> I'd say having it as a flag/bit on the DICompileUnit's probably the way to go - and it can be consulted whenever choices about a particular Function are being made.

Don’t we merge DICompileUnit? Like if a.cpp and b.cpp includes the same header: will the DICompileUnit for this header be merged? If so what if the flag mismatch? Do we want it to prevent merging for instance?
(I have really no idea the consequences, so these are really naive questions right now).

> When it comes to the discriimnator features that will be associated with this flag soon maybe that gets a bit trickier - can they be respected once inlining occurs? (eg: a -fdebug-info-for-profiling function inlined into a plain/non-debug-info-for-profiling function, can the bits of the inlined function have all the good stuff but the outer function can still get the normal treatment? Probably? maybe?)
> 
> 
> If the decision goes the other way (use a module flag) then it's probably fairly simple - the merge choice is probably "if any module has -fdebug-info-for-profiling, the resulting module does".
> 
> 
> One extra wrinkle: ThinLTO doesn't merge modules, it imports functions from them. Does it import/resolve conflicts in module flags too? If not, it probably should (even for the exsiting module flags, regardless of this new one (whether or not it becomes a module flag or not))

We materialize the module flags, and we run the IRLinker, I’m not sure if the logic to resolve conflicts is honored the same way (I hope so).

— 
Mehdi

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170127/5fac7437/attachment.html>