[PATCH] Move the IsIndirect flag from DIVariable into DIExpression.

[Adrian Prantl]

> On Jan 16, 2015, at 11:13 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Fri, Jan 16, 2015 at 10:52 AM, Adrian Prantl <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
> In http://reviews.llvm.org/D6986#109826 <http://reviews.llvm.org/D6986#109826>, @dblaikie wrote:
> 
> > (might be easier to review/commit by adding the expression handling then just removing the flag as dead code afterwards, but I'm not sure - it's not bad the way you've got it either)
> 
> 
> Oh that would be great, but it's complicated:
> After having read this patch, look at this gem from SelectionDAGBuilder:
> 
>   uint64_t Offset = DI->getOffset();
>     // A dbg.value for an alloca is always indirect.
>     bool IsIndirect = isa<AllocaInst>(V) || Offset != 0;
>     SDDbgValue *SDV;
>     if (Val.getNode()) {
>       if (!EmitFuncArgumentDbgValue(V, Variable, Expr, Offset, IsIndirect,
>                                     Val)) {
>         SDV = DAG.getDbgValue(Variable, Expr, Val.getNode(), Val.getResNo(),
>                               IsIndirect, Offset, dl, DbgSDNodeOrder);
>         DAG.AddDbgValue(SDV, Val.getNode(), false);
>       }
>     } else
> 
> It is rematerializing the OP_deref that this patch removes in the form a of an indirect MachineLocation.
> 
> Horrible, right?
> 
> OK, so the indirect flag on MachineLocation is separate/in addition to the indirect flag on DIVariable that gets passed around through a few other layers/side channels.

Yes.

> 
> So you could still remove the DIVariable related indirection and keep the MachineLocation indirection for now, maybe? Not sure.

We definitely can do this — in fact, that’s what this patch is doing.

> Maybe they end up together eventually.
>  
> We //could// get rid of the indirect field in MachineLocation, if we were to redefine alloca's to be treated as addresses rather than values in dbg.value/dbg.declare.
> Let me give an example. Currently, the following code:
> 
>   %x = alloca i64
>   call void @llvm.dbg.declare(metadata %i64* %b, [var=x], [expr=[]])
>   store i64 42, %x
> 
> is lowered into
> 
>   %x = i64 42
>   call void @llvm.dbg.value(metadata %i64* %b, [var=x], [expr=[]])
> 
> Observe that the alloca, although it is treated like an address in IR, is treated like it was the value as far as the debug info is concerned.
> If we want to fix this we'd have to change the frontend to emit
> 
>   %x = alloca i64
>   call void @llvm.dbg.declare(metadata %i64* %b, [var=x], [expr=[DW_OP_deref]])
>   store i64 42, %x
> 
> which could then be lowered into the same code as above. After separating out DIExpression from DIVariable this is really easy to do; before it was pretty much impossible.
> 
> I think I'd love to do that instead. Should we go break stuff? ;-)
> 
> I believe this is the right direction to go sooner or later (Eric - can you confirm/deny?).

I’d be happy to prepare a patch that does that. Such a patch would be orthogonal to this patch (D6986), but it would obsolete D7016.

> This would get us pretty close to just removing dbg.declare entirely, wouldn't it? Or am I misremembering/misunderstanding the distinction.

Implementation-wise there’s more work left to be done. DbgDeclareInsts are handled in a completely independent pipeline from SDag down — they are tracked in the MMI side table and are not represented in the MachineFunctions as such.

> It'd just be a matter of whether we are indirecting through a register that doesn't change or not. (modulo lifetime intrinsics that make this harder - but they shouldn't shrink variable lifetime beyond the lexical scope anyway... maybe? so perhaps they don't matter)

It would also allow us to get a more accurate lifetime for stack variables (which currently are by definition valid throughout their entire scope). But in order for this to work, we’d need a more powerful DbgValueHistoryCalculator (which Fred is determined to tackle) or have LiveDebugVariables insert dbg.values into every basic block.

-- adrian
>  
> 
> 
> REPOSITORY
>   rL LLVM
> 
> http://reviews.llvm.org/D6986 <http://reviews.llvm.org/D6986>
> 
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/ <http://reviews.llvm.org/settings/panel/emailpreferences/>
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150116/369370ac/attachment.html>