[PATCH] D11933: Extend debug ranges and provide multiple location support for debug variables

[Vikram TV via llvm-commits]

tvvikram added a comment.

In http://reviews.llvm.org/D11933#221911, @friss wrote:

> > This patch is an early implementation to extend debug ranges and provide multiple location support for debug variables. This work is based on the idea put forward in the mail thread: http://lists.cs.uiuc.edu/pipermail/llvmdev/2015-June/087109.html
>
>
> Thank you so much for working on this! I just skimmed through the patch and description. Some questions bellow:
>
> > A. Debug Range Extension: Currently, the debug ranges in LLVM end prematurely at the end of every basic block. This patch tries to extend them across basic blocks such that when two predecessors of a basic block have the same variable residing in the same location, a new debug value representing the variable and the location is added at the start of the current basic block.
>
>
> I think there is an interesting corner case here. I believe that constant values should have a special treatment. When you have code like:
>
> int i = 42;
>  while (i)  i =foo(i)
>
> the DBG_VALUE for i will have a constant on one edge into the loop and a register on the other one. In that scenario, in case a constant competes with a dynamic location at BB entry, I strongly believe the dynamic loc should win.


Currently, both are treated as separate locations and we do nothing about it.

> Also, was it really easier to write a different pass to do the data flow propagation? I saw in the patch that it is because you want to insert new instructions, but couldn't it be done without inserting the instructions, just keeping some temporary data structures up-to-date? I can see some appeal in minimally modifying the current code and reviewing the new pass as a different thing though.


buildLocationLists() need ranges with real instructions. So we had to insert a new instruction.

> 

> 

> > B. Multiple locations: LLVM currently can represent a debug variable only at a single location. This patch provides an infrastructure for earlier passes to express multiple locations for a debug variable by adding a new flag - PreserveDbgValLoc to the DBG_VALUE instruction. The presence of the flag will mean multiple debug ranges for the variable can co-exist together. The patch also infers multiple locations by looking at move instructions.

> 

> 

> Could that be split, maybe even multiple times (split out of this patch, then the move inference part)? Why is the flag needed rather than tracking multiple locations unconditionally.


Flag is provided so that prior passes can set it to convey that a variable exists at a location along with previous locations, if any.

> One thing that we discussed with Adrian some time ago is that emitting multiple locations isn't really useful for the debuggers we now, they will just use one location. Thus the generic code shouldn't maybe emit multiple locations as it will cost file size and IO time for nothing.

> 

> > Example - inferring from move instruction: Consider the following two instructions:

> 

> > 

> 

> >   DBG_VALUE %EDI, %noreg, !"n", <!12>

> 

> >   %EBX<def> = MOV32rr %EDI

> 

> >   DBG_VALUE %EBX, %noreg, !"n", <!12>; flags: PreserveDbgValLoc  <-- newly inserted

> 

> > 

> 

> > After the copy instruction, the value “n” will be present in two locations - %EDI and %EBX and the same is represented with the new DBG_VALUE instruction having the flag PreserveDbgValLoc set.

> 

> > 

> 

> > The two ranges continue to exist together until one of the following happens:

> 

> >  Either %EDI or %EBX is clobbered, the two ranges are conservatively ended

> 

> >  A DBG_VALUE instruction for variable “n” without the flag is seen, when the previous ranges are ended.

> 

> > 

> 

> > Test cases: 3 new test cases are added to test the above implementations. There are numerous places in the test cases where debug range extension and addition of PreserveDbgValLoc happens. All test cases under 'ninja check' run, with around 5 miscompare errors. I will correct them once the implementation is okay as the miscompares are volatile with code change.

> 

> > 

> 

> > SVN base revision used: 243841

> 

> 

> One thing I'm missing is results. You propose the patch, so I suppose this approach works for you. Could you give some details on what works better, eg does it fix all the issues you were seeing? Also, what's the compile time impact of that change and the effect on binary size?

> 

> Fred





http://reviews.llvm.org/D11933