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

[David Blaikie]

On Fri, Jan 16, 2015 at 11:31 AM, Adrian Prantl <aprantl at apple.com> wrote:

>
> 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> wrote:
>
>> In 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.
>

Right, sorry, looping back up to my original question, though it's not
necessary by any means - why would removing the DIVariable "indirect" flag
as a separate commit be problematic? (essentially leave all the code in but
change clang/DIBuilder to never use the indirect flag but instead use an
indirect expression - then, remove the DIVariable indirect flag)


>
> 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.
>

Yeah - all of those different side tables & paths... :/ Any idea what the
differences/features of each option are? I guess the dbg_declare stuff is
good for tracking stack frames and the MMI side tables are good for
tracking something else, but doesn't give assurances that the expression is
valid for the whole function. Beats me.


>
> 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
>

"it would" - what's "it" in this sentence - removing/coalescing dbg.declare
and dbg.value? Not quite sure how it would provide these \/ benefits, but
I'm not sure it wouldn't either.


> (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
>>
>> EMAIL PREFERENCES
>>   http://reviews.llvm.org/settings/panel/emailpreferences/
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150116/64a95951/attachment.html>