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

[David Blaikie via llvm-commits]

On Fri, Jan 27, 2017 at 11:35 AM Mehdi Amini <mehdi.amini at apple.com> wrote:

> 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> 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?
>

There is no DICompileUnit for the header. There's only a DICompileUnit for
a.cpp and one for b.cpp.

Say both have an instance of the same inline function.

In a non-LTO build both the CUs would have a description of the function,
and both descriptions would point to their respective addresses in the
object files. Once linked, they'd both end up pointing to the same code (so
you'll have two descriptions of the same function pointing at the same code
- heaven forbid you optimized those differently and one of your
descriptions says the variable X is in memory and the other says the
variable X is in a register... ;) )

In an LTO build the llvm::Function points to the DISubprogram which points
to the CU. When merging modules, one of the llvm::Functions is dropped.
Once that happens the debug info no longer appears in the CU of the
Function that was dropped. So one CU has no
description/subprogram/function, the other CU does have one.


> 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).
>

*fingers crossed* :)


>
> —
> Mehdi
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170127/9f770488/attachment.html>