[PATCH] Move the IsIndirect flag from DIVariable into DIExpression.
Adrian Prantl
aprantl at apple.com
Mon Jan 19 09:45:28 PST 2015
> On Jan 17, 2015, at 2:37 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Fri, Jan 16, 2015 at 4:59 PM, Adrian Prantl <aprantl at apple.com <mailto: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
Heh, I just made an interesting observation: Since moving from the Indirect flag to the DIExpression is a CFE-only change; it actually *is* split up into two commits.
-- adrian
>
>
> -- adrian
>
> On Jan 16, 2015, at 12:12 PM, Adrian Prantl <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
>>
>>
>>> On Jan 16, 2015, at 12:00 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>>
>>>
>>>
>>> On Fri, Jan 16, 2015 at 11:31 AM, Adrian Prantl <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
>>>
>>>> On Jan 16, 2015, at 11:13 AM, David Blaikie <dblaikie at gmail.com <mailto: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.
>>>
>>> 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 <http://reviews.llvm.org/D6986>
>>>>
>>>> EMAIL PREFERENCES
>>>> http://reviews.llvm.org/settings/panel/emailpreferences/ <http://reviews.llvm.org/settings/panel/emailpreferences/>
>>>>
>>>>
>>>>
>>>
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <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/20150119/731f195e/attachment.html>
More information about the llvm-commits
mailing list