<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 16, 2015 at 11:31 AM, Adrian Prantl <span dir="ltr"><<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><span class=""><blockquote type="cite"><div>On Jan 16, 2015, at 11:13 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 16, 2015 at 10:52 AM, Adrian Prantl <span dir="ltr"><<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>In <a href="http://reviews.llvm.org/D6986#109826" target="_blank">http://reviews.llvm.org/D6986#109826</a>, @dblaikie wrote:<br>
<br>
> (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>
<br>
<br>
</span>Oh that would be great, but it's complicated:<br>
After having read this patch, look at this gem from SelectionDAGBuilder:<br>
<br>
uint64_t Offset = DI->getOffset();<br>
// A dbg.value for an alloca is always indirect.<br>
bool IsIndirect = isa<AllocaInst>(V) || Offset != 0;<br>
SDDbgValue *SDV;<br>
if (Val.getNode()) {<br>
if (!EmitFuncArgumentDbgValue(V, Variable, Expr, Offset, IsIndirect,<br>
Val)) {<br>
SDV = DAG.getDbgValue(Variable, Expr, Val.getNode(), Val.getResNo(),<br>
IsIndirect, Offset, dl, DbgSDNodeOrder);<br>
DAG.AddDbgValue(SDV, Val.getNode(), false);<br>
}<br>
} else<br>
<br>
It is rematerializing the OP_deref that this patch removes in the form a of an indirect MachineLocation.<br>
<br>
Horrible, right?<br></blockquote><div><br>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></div></div></div></div></div></blockquote><div><br></div></span><div>Yes.</div><span class=""><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>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></div></span><div>We definitely can do this — in fact, that’s what this patch is doing.</div></div></div></blockquote><div><br>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)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><span class=""><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Maybe they end up together eventually.<br> </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>
Let me give an example. Currently, the following code:<br>
<br>
%x = alloca i64<br>
call void @llvm.dbg.declare(metadata %i64* %b, [var=x], [expr=[]])<br>
store i64 42, %x<br>
<br>
is lowered into<br>
<br>
%x = i64 42<br>
call void @llvm.dbg.value(metadata %i64* %b, [var=x], [expr=[]])<br>
<br>
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>
If we want to fix this we'd have to change the frontend to emit<br>
<br>
%x = alloca i64<br>
call void @llvm.dbg.declare(metadata %i64* %b, [var=x], [expr=[DW_OP_deref]])<br>
store i64 42, %x<br>
<br>
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>
<br>
I think I'd love to do that instead. Should we go break stuff? ;-)<br></blockquote><div><br>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></div></span><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><span class=""><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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></div></span><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></div></div></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><span class=""><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> 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></div></div></div></div></div></blockquote><div><br></div></span>It would also allow us to get a more accurate lifetime for stack variables</div></div></blockquote><div><br>"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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div> (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><span class="HOEnZb"><font color="#888888"><div><br></div></font></span><div><span class="HOEnZb"><font color="#888888">-- adrian</font></span><span class=""><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
<br>
REPOSITORY<br>
rL LLVM<br>
<br>
<a href="http://reviews.llvm.org/D6986" target="_blank">http://reviews.llvm.org/D6986</a><br>
<br>
</span>EMAIL PREFERENCES<br>
<a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
</blockquote></div><br></div></div>
</div></blockquote></span></div><br></div></blockquote></div><br></div></div>