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

Sat Jan 17 14:37:32 PST 2015

On Fri, Jan 16, 2015 at 4:59 PM, Adrian Prantl <aprantl at apple.com> wrote:

> on my end the only question that remains is: should we leave a hole in the
> enum, or is it ok reassign the higher enumerators to close the gap?
>

I wouldn't bother with it (again, could be a two-step to make it easier to
read the patches, but up to you).

Go ahead

>
> -- adrian
>
> On Jan 16, 2015, at 12:12 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
>
>
> On Jan 16, 2015, at 12:00 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> 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)
>
>
> There’s no problem with doing that, it just seemed like a pointless
> intermediate step to me. If you want, I can split the patches up.
>
>
>
>>
>> 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.
>
>
> There are only two paths for debug locations: DBG_VALUE machine
> instructions (what dbg.values get lowered to) and the MMI side table (where
> dbg.declares are being tracked).
> The advantage of having the expression separate is that we can create as
> many expressions as we want; while it is not possible to modify/clone a
> DIVariable without making a mess.
>
>
>
>>
>> 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?
>
> Yes, sorry!
>
> Not quite sure how it would provide these \/ benefits, but I'm not sure it
> wouldn't either.
>
>
> See also my above comment about how locations are handled: DBG_VALUEs more
> or less automatically track the lifetime (via DbgValueHistoryCalculator and
> friends), whereas an MMI table entry has no concept of liveness and is
> always valid for the entire lexical scope.
>
> -- adrian
>
>
>
>> (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/
>>>
>>>
>>>
>>
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150117/de60ef47/attachment.html>