[PATCH] D28075: MergeFunctions: Preserve debug info in thunks, under option -mergefunc-preserve-debug-info

[Anmol P. Paralkar via Phabricator via llvm-commits]

appcs added a comment.

In https://reviews.llvm.org/D28075#637947, @aprantl wrote:

> In https://reviews.llvm.org/D28075#637667, @appcs wrote:
>
> > In https://reviews.llvm.org/D28075#637188, @aprantl wrote:
> >
> > > > Call sites of the merged function occurring from within the TU of the merged function's definition will point directly to the shared implementation and call sites of the merged function that are external to the TU of the merged function's definition call the thunk for the merged function (which tail call's the shared implementation passing forward the incoming arguments). This is existing behaviour under -mergefunc and remains unchanged under -mergefunc-preserve-debug-info (except for the tail call part).
> > >
> > > Is this the right thing to do, though? I would think that for better debugability users would always prefer the version with the thunk even in the current TU. Then again, subsequent optimization passes would likely inline the thunk and thus potentially undo most of the effect. Have you experimented with this variant? Would the debug info from the thunk survive?
> >
> >
> > Thank you; I agree that this greatly enhances the debug experience. Automatically modifying the call site with a direct call to the shared implementation instead of (what is now) the thunk, when within the TU, is confusing when debugging. Stepping into the thunk from the call site and from within the thunk, into the shared implementation keeps the debug experience uniform for internal and external call sites of functions that become thunks. (If you ask me, this is similar in spirit to not marking the thunk's call to the shared implementation as a tail call to help debugging - the full back trace is the same in both cases and otherwise, there is a certain element of surprise in both cases. Basically, we are saying that under -mergefunc-preserve-debug-info we trade off optimization partly (at -O0) to aid debugability, modifying the transformation slightly, to make the execution flow explicit).
>
>
> Yes. I think this is the right approach. When using -mergefunc-preserve-debug-info we should always call the thunk even in the same TU.


Thank you for confirming.

What should -mergefunc-preserve-debug-info do about the thunk's call to the shared implementation?
 Mark as tail call or not? The existing -mergefunc behaviour is to mark it as a tail call. We could leave it as is,
 unless someone specifically asks for a change under -mergefunc-preserve-debug-info; would that be OK?

> 
> 
>> The same question remains for thunk call sites within the TU; do we do this:
>> 
>> [debug]         At -O0                   -mergefunc -mergefunc-preserve-debug-info - call thunk even though within the same TU
> 
> Is this a scenario that anyone would use in practice, or did you just add that for comparison?

I have not added it yet; I'm thinking that for now, we'll not add the optimization level to the consideration under -mergefunc-preserve-debug-info unless some real user specifically asks for it. Does that sound reasonable?

>> [production] At -O<non-zero> -mergefunc -mergefunc-preserve-debug-info - call shared implementation [as-(currently)-is]
>> 
>> MergeFunctions runs pretty much at the end of the sequence of optimization phases (only prior to Loop sinking and Instruction Simplify), so inlining has already happened at the call-sites.
> 
> Oh interesting. I (never having studied the PassManager before) was expecting the transformations to run in some kind of fix-point iteration.

I was looking at lib/Transforms/IPO/PassManagerBuilder.cpp/PassManagerBuilder::populateModulePassManager() 
 and saw it listed immediately above those passes, but I was not aware of:
 -debug-pass=Structure                                             -   print pass structure before run()
 which lists Merge Functions running pretty much as one of the last passes at -O2, but the tail bit is:

  Merge Functions                                                                   
  Print module to stderr                                                            
  FunctionPass Manager                                                              
    Module Verifier                                                                 
    Print function to stderr                                                        
  Bitcode Writer                       

I found that http://llvm.org/docs/CommandGuide/opt.html says:
 "The order in which the options occur on the command line are the order in which they are executed (within pass constraints)."
 so, that is actually how I got -inline to run after -mergefunc for the experiment.

- Thank you.


https://reviews.llvm.org/D28075