<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jan 16, 2015, at 11:13 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Fri, Jan 16, 2015 at 10:52 AM, Adrian Prantl <span dir="ltr" class=""><<a href="mailto:aprantl@apple.com" target="_blank" class="">aprantl@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">In <a href="http://reviews.llvm.org/D6986#109826" target="_blank" class="">http://reviews.llvm.org/D6986#109826</a>, @dblaikie wrote:<br class="">
<br class="">
> (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)<br class="">
<br class="">
<br class="">
</span>Oh that would be great, but it's complicated:<br class="">
After having read this patch, look at this gem from SelectionDAGBuilder:<br class="">
<br class="">
uint64_t Offset = DI->getOffset();<br class="">
// A dbg.value for an alloca is always indirect.<br class="">
bool IsIndirect = isa<AllocaInst>(V) || Offset != 0;<br class="">
SDDbgValue *SDV;<br class="">
if (Val.getNode()) {<br class="">
if (!EmitFuncArgumentDbgValue(V, Variable, Expr, Offset, IsIndirect,<br class="">
Val)) {<br class="">
SDV = DAG.getDbgValue(Variable, Expr, Val.getNode(), Val.getResNo(),<br class="">
IsIndirect, Offset, dl, DbgSDNodeOrder);<br class="">
DAG.AddDbgValue(SDV, Val.getNode(), false);<br class="">
}<br class="">
} else<br class="">
<br class="">
It is rematerializing the OP_deref that this patch removes in the form a of an indirect MachineLocation.<br class="">
<br class="">
Horrible, right?<br class=""></blockquote><div class=""><br class="">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.<br class=""></div></div></div></div></div></blockquote><div><br class=""></div><div>Yes.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class="">So you could still remove the DIVariable related indirection and keep the MachineLocation indirection for now, maybe? Not sure.</div></div></div></div></div></blockquote><div><br class=""></div><div>We definitely can do this — in fact, that’s what this patch is doing.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">Maybe they end up together eventually.<br class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br class="">
Let me give an example. Currently, the following code:<br class="">
<br class="">
%x = alloca i64<br class="">
call void @llvm.dbg.declare(metadata %i64* %b, [var=x], [expr=[]])<br class="">
store i64 42, %x<br class="">
<br class="">
is lowered into<br class="">
<br class="">
%x = i64 42<br class="">
call void @llvm.dbg.value(metadata %i64* %b, [var=x], [expr=[]])<br class="">
<br class="">
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.<br class="">
If we want to fix this we'd have to change the frontend to emit<br class="">
<br class="">
%x = alloca i64<br class="">
call void @llvm.dbg.declare(metadata %i64* %b, [var=x], [expr=[DW_OP_deref]])<br class="">
store i64 42, %x<br class="">
<br class="">
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.<br class="">
<br class="">
I think I'd love to do that instead. Should we go break stuff? ;-)<br class=""></blockquote><div class=""><br class="">I believe this is the right direction to go sooner or later (Eric - can you confirm/deny?). </div></div></div></div></div></blockquote><div><br class=""></div><div>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.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">This would get us pretty close to just removing dbg.declare entirely, wouldn't it? Or am I misremembering/misunderstanding the distinction.</div></div></div></div></div></blockquote><div><br class=""></div><div>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.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> 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)<br class=""></div></div></div></div></div></blockquote><div><br class=""></div>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.</div><div><br class=""></div><div>-- adrian<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br class="">
<br class="">
REPOSITORY<br class="">
rL LLVM<br class="">
<br class="">
<a href="http://reviews.llvm.org/D6986" target="_blank" class="">http://reviews.llvm.org/D6986</a><br class="">
<br class="">
</span>EMAIL PREFERENCES<br class="">
<a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank" class="">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br class="">
<br class="">
<br class="">
</blockquote></div><br class=""></div></div>
</div></blockquote></div><br class=""></body></html>