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

Fri Jan 27 11:57:35 PST 2017

On Fri, Jan 27, 2017 at 11:54 AM Dehao Chen <danielcdh at gmail.com> wrote:

> On Fri, Jan 27, 2017 at 11:45 AM, David Blaikie <dblaikie at gmail.com>
> wrote:
>
>
>
> 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 a.cc and b.cc both includes c.h, which has foo() defined. If a.cc is
> built with -g and b.cc is built with -gmlt:
>
> * will LTO pick foo built with "-g" or "-gmlt"
>

I'm not sure of the specific logic that the LTO linker uses, but
essentially "arbitrary" (I suspect it's based on the order of the arguments
- I think we merge things into the first module, so whichever comes first
will keep its inline definition and others will be discarded)

> * will nonLTO linker think this is ODR violation? if not, which one would
> the linker choose
>

Non LTO linking won't know the difference between the two (it doesn't look
at/understand debug info at all, really) & doesn't detect ODR violations
anyway (well, there's that flag... but that's a more complicated thing
where it does look at debug info - we've turned that off due to
incompatibilities between GCC and Clang for debug info).

The linker would choose arbitrarily & any debug info for each of the
original descirpitons will point to the one chosen by the linker.

>
> Dehao
>
>
>
>
> 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/d6e9e1d8/attachment.html>