<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 12:12 PM, Adrian Prantl <<a href="mailto:aprantl@apple.com" class="">aprantl@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" class=""><div class=""><br class="Apple-interchange-newline">On Jan 16, 2015, at 12:00 PM, 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 11:31 AM, Adrian Prantl<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:aprantl@apple.com" target="_blank" class="">aprantl@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div class="" style="word-wrap: break-word;"><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Jan 16, 2015, at 11:13 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:</div><br class=""><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 class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:aprantl@apple.com" target="_blank" class="">aprantl@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><span class="">In<span class="Apple-converted-space"> </span><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 class=""><br class=""></div></span><div class="">Yes.</div><span class=""><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 class=""><br class=""></div></span><div class="">We definitely can do this — in fact, that’s what this patch is doing.</div></div></div></blockquote><div class=""><br class="">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 class=""></div></div></div></div></div></blockquote><div class=""><br class=""></div>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.<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: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div class="" style="word-wrap: break-word;"><div class=""><span class=""><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: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: 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?).<span class="Apple-converted-space"> </span></div></div></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">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 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 class=""><br class=""></div></span><div class="">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 class=""><br class="">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 class=""></div></div></div></div></div></blockquote><div class=""><br class=""></div><div class="">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).</div><div class="">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.</div><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: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div class="" style="word-wrap: break-word;"><div class=""><span class=""><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 class=""><br class=""></div></span>It would also allow us to get a more accurate lifetime for stack variables</div></div></blockquote><div class=""><br class="">"it would" - what's "it" in this sentence - removing/coalescing dbg.declare and dbg.value?</div></div></div></div></div></blockquote>Yes, sorry!<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">Not quite sure how it would provide these \/ benefits, but I'm not sure it wouldn't either.<br class=""></div></div></div></div></div></blockquote><div class=""><br class=""></div>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.</div></div></blockquote><div><br class=""></div><div>As Adrian pointed out I plan to work on DbgValueHistoryCalculator to improve the optimized code location info generation (it also triggers for some corner cases of non-optimized code). I am not sure that it will make it possible to totally get rid of the MMI table (but it would definitely be a required step to go that way). This isn’t news to anybody reading this thread, but we are able to debug today at -O0 because of the MMI table tracking the allocas. The debugging experience would be rather sad without that I fear.</div><div><br class=""></div><div>The end goal of getting reading of one of the 2 ways the location information flows through the compiler is certainly worthwhile, I just wanted to point out that it looks like a pretty far perspective to me :-) </div><div><br class=""></div><div>Fred</div><div><br class=""></div><blockquote type="cite" class=""><div class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">-- 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: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div class="" style="word-wrap: break-word;"><div class="">(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" class=""><div class=""><br class=""></div></font></span><div class=""><span class="HOEnZb"><font color="#888888" class="">-- adrian</font></span><span class=""><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: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: 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=""> <span class="Apple-converted-space"> </span><a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank" class="">http://reviews.llvm.org/settings/panel/emailpreferences/</a></blockquote></div></div></div></div></blockquote></span></div></div></blockquote></div></div></div></div></blockquote></div></div></blockquote></div><br class=""></body></html>