<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 Apr 6, 2016, at 2:56 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 Wed, Apr 6, 2016 at 8:44 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"><div style="word-wrap:break-word" class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Apr 6, 2016, at 8:16 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:</div><br class=""><div class=""><br class=""><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">On Tue, Apr 5, 2016 at 10:17 PM, Eric Christopher via llvm-commits<span class=""> </span><span dir="ltr" class=""><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>></span><span class=""> </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">echristo added inline comments.<br class=""><br class="">================<br class="">Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:477-479<br class="">@@ -476,2 +476,5 @@<br class="">+ unsigned DebugCUs = 0;<br class=""> for (MDNode *N : CU_Nodes->operands()) {<br class=""> auto *CUNode = cast<DICompileUnit>(N);<br class="">+ if (CUNode->getEmissionKind() == DICompileUnit::NoDebug)<br class="">+ continue;<br class="">----------------<br class="">Instead of this pattern would it make more sense to have an iterator over the nodes that checks for !NoDebug?<br class=""></blockquote><div class=""><br class=""></div><div class="">Yeah, that sounds like something that might be nice.<br class=""><br class="">Adrian - I recall one of your initial ideas was to just have a different named metadata node for these subprograms. </div></div></div></blockquote><div class=""><br class=""></div></span><div class="">Huh. I think I had this idea in the context of collectDeadVariables (to collect dead subprograms), but I see how it would apply here, too.</div><span class=""><div class=""><div class=""><br class=""></div></div><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="">What was the motivation for not going with that plan? (which would've meant these sort of loops would've remained OK, and only the loops in the verifier and such would need to examine both nodes?) Perhaps this relates to your comment about LTOing debug+nodebug CUs? I didn't quite follow what was wrong with that in the first place & how this addresses it, could you explain it?<br class=""></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">Most of the patches I’ve been committing recently are in preparation reversing the ownership between DICompileUnit and DISubprogram (which will help ThinLTO to lazy-load debug info).</div><div class="">DICompileUnit will no longer have a list of “subprograms:" and instead each DISubprogram will have a “unit:" field that points to the owning CU.</div><div class=""><br class=""></div><div class="">While it would be possible to leave the unit field empty and have a NamedMDNode hold on to these subprograms, having a mandatory “unit:" point to a CU that is explicitly marked NoDebug is a more regular and readable representation. It’s straightforward to verify and easier to debug especially in the LTO case.</div></div></div></blockquote><div class=""><br class=""></div><div class="">Not sure whether it adds much more regularity or irregularity compared to having a null unit for the subprogram, really?<br class=""></div></div></div></div></div></blockquote><div><br class=""></div><div>My thinking was that we could then have to easily enforceable rules:</div><div><br class=""></div><div>1. The DISubprograms that are part of the type hierarchy (uniqued, and isDefinition: false) *never* have a unit field.</div><div>2. The DISubprogram definitions (distinct) *must* have a unit field. </div><div><br class=""></div><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">Anyone looking at the subprogram for debug purposes is going to have to walk up and check that it's not a nodebug CU, the CU isn't really a CU (when looking at the code/IR/etc - it'll be a weird construct)... but I dunno.</div></div></div></div></div></blockquote><div><br class=""></div><div>After the pointer reversal we can add a DISubprogram::isNoDebug() convenience accessor. For the runtime performance the extra indirection doesn’t matter because in each of these places we already look up the parent CU.</div><div><br class=""></div><div>From a practical point of view, implementing the null unit option is actually quite a bit more work, given how much CGDebugInfo and DIBuilder currently depend on having a CU. With enough effort this can be cleaned up as well and this patch doesn’t prevent us from doing this in the future.</div><div><br class=""></div><div>-- adrian</div><div><br class=""></div><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"><div style="word-wrap:break-word" class=""><div class=""><span class="HOEnZb"><font color="#888888" class=""><div class=""><br class=""></div><div class="">-- adrian</div></font></span><span class=""><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><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"><br class="">================<br class="">Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1127-1128<br class="">@@ -1115,3 +1126,4 @@<br class=""> if (!MMI->hasDebugInfo() || LScopes.empty() ||<br class="">- !MF->getFunction()->getSubprogram()) {<br class="">+ !MF->getFunction()->getSubprogram() ||<br class="">+ !SPMap.lookup(MF->getFunction()->getSubprogram())) {<br class=""> // If we don't have a lexical scope for this function then there will<br class="">----------------<br class="">Comment.<br class=""><span class=""><br class=""><br class="">Repository:<br class=""> <span class=""> </span>rL LLVM<br class=""><br class=""><a href="http://reviews.llvm.org/D18808" rel="noreferrer" target="_blank" class="">http://reviews.llvm.org/D18808</a><br class=""><br class=""><br class=""><br class=""></span><div class=""><div class="">_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a><br class=""><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a></div></div></blockquote></div></div></blockquote></span></div><br class=""></div></blockquote></div><br class=""></div></div>
</div></blockquote></div><br class=""></body></html>