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

- Dave


>
>
> https://reviews.llvm.org/D29203
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170127/f3f55290/attachment.html>