[llvm] [DebugInfo] Preserve line and column number when merging debug info. (PR #129960)

[Snehasish Kumar via llvm-commits]

snehasish wrote:

Thanks for taking a look @dwblaikie.

> This seems like a pretty substantial departure from LLVM's documented approach to instruction line information - a policy that was established predominantly as an agreement/unified needs between Google (for profile accuracy) and Sony (for user debuggability/not so "jumpy" stepping).

We do not intend to change the behaviour for scenarios where users are stepping through the binary. This behaviour is intended to be enabled in production where sample profiles are collected, similar in spirit to `-fdebug-info-for-profiling`. This is the reason why we will keep this gated behind a flag.

> So this deserves a bit more discussion to better understand why we're considering departing from that established decision now.
> 
> For instance, this seems like it does the wrong thing for PGO, as far I've understood the goals, for an example like this:
> 
> ```
> void f1();
> void f2();
> void f3();
> void f4(bool b) {
>   if (b) {
>     f1();
>     f2();
>   } else {
>     f1();
>     f3();
>   }
> }
> ```
> 
> The call to `f1()` is hoisted out of the if/else, and currently would be given a source location of line zero - whereas with this change, it receives line 6 (the first of the two calls).
> 
> The premise so far has been that that change is harmful for users (they might incorrectly conclude that since line 6 is reached, then `b` must be true, even when it is false) and for profile-guided optimizations (for similar reasons - the compiler might assume that `b` is true, that the `f2` call is similarly hot, and the `f3` call is cold).
> 
> Could you provide more context for how this ^ benefits PGO? Or which cases do benefit?

I believe this notion of attribution for AutoFDO is outdated. Today hotness inference for a basic block relies on branch target information. In your example, we would use the debug information attached to the if statement to determine the hotness of the callsites for f2 and f3. 

> Or which cases do benefit?

Debug info loss on merged indirect calls is particularly important since it disallows indirect call promotion. The net impact of retaining a deterministic version of merged debug info for a large workload is positive. MemProf is a context sensitive profile which is particularly susceptible to loss of debug information on direct and indirect calls. This is part of a broader effort to improve profile quality for AutoFDO and MemProf by addressing missing debug info. I think we will find additional opportunities and once we have a better idea, I can update the documentation and guidance at https://llvm.org/docs/HowToUpdateDebugInfo.html. 

Does that sound reasonable to you?

https://github.com/llvm/llvm-project/pull/129960