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

Fri Jan 27 11:54:53 PST 2017

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"
* will nonLTO linker think this is ODR violation? if not, which one would
the linker choose

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