<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Dec 15, 2016 at 10:16 AM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div dir="ltr" class="gmail_msg">On Thu, Dec 15, 2016 at 10:05 AM Adrian Prantl via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="gmail_msg" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br class="gmail_msg">
> On Dec 14, 2016, at 9:10 PM, David Blaikie via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="gmail_msg" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br class="gmail_msg">
><br class="gmail_msg">
> dblaikie added inline comments.<br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
> ================<br class="gmail_msg">
> Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:500<br class="gmail_msg">
> +      GVMap[GVE->getVariable()].push_back({nullptr, GVE->getExpression()});<br class="gmail_msg">
> +    DenseSet<DIGlobalVariable *> Processed;<br class="gmail_msg">
> +    for (auto *GVE : CUNode->getGlobalVariables()) {<br class="gmail_msg">
> ----------------<br class="gmail_msg">
> (terminology:<br class="gmail_msg">
> First loop: the "for globals" loop outside the CU loop. This one finds/associates GVEs on actual globals.<br class="gmail_msg">
> Second loop: the first "for global variables" loop inside the CU loop. This one finds any GVEs over constants (not associated with globals) - does the CU GV list not contain the GVEs associated with actual globals? (if it does contain those, then we might have duplicates in the first and second loops - which seems unfortunate)<br class="gmail_msg">
> Third loop: the second "for global variables" loop inside the CU loop - this one creates the global variables.<br class="gmail_msg">
><br class="gmail_msg">
> Should this be scoped outside of the CU loop?<br class="gmail_msg">
><br class="gmail_msg">
> (if the same global variable (eg: static in an inline function) is defined in multiple translation units - we only really need to emit it in one of them, not duplicate the description in all of them, right?)<br class="gmail_msg">
><br class="gmail_msg">
> Also - adding more things to the GVMap inside CU loop then processing the contents seems a bit strange - at best  (& I think) benign, at worst one CU could leak something into another? (& likely somewhere in between: performance degredation by the final CU having many more things in the map than it needs, just making lookups slow, etc)<br class="gmail_msg">
><br class="gmail_msg">
> I think maybe the Processed set needs to move out of the CU loop, and we should remove entries from the GVMap in the Third Loop below (essentially migrating things out of the GVMap once they're handled - but recording that fact in the Processed (we can't just take absence as an indication because it might be absent because it's been optimized away to constants, so it wouldn't appear until the Second Loop - we shouldn't add things in the Second Loop if they're already in Processed, perhaps? Then we wouldn't need a conditional in the third loop?)<br class="gmail_msg">
><br class="gmail_msg">
> Oh, the 3rd loop could be over the GVMap, filtered by whether the unit is the current one. Which could be more efficient if there are many fragments of variables - which I guess is not teh common case, so the current pivot's probably better.<br class="gmail_msg">
><br class="gmail_msg">
> There do seem like an awful lot of lookups in many directions in these three loops. Hrm. :/<br class="gmail_msg">
><br class="gmail_msg">
> Maybe if we started off CU-specific? (the First Loop could create a map from CU -> GV -> GVEs,<br class="gmail_msg">
<br class="gmail_msg">
Are you thinking of a<br class="gmail_msg">
<br class="gmail_msg">
  DenseMap<DICompileUnit *,<br class="gmail_msg">
    DenseMap<DIGlobalVariable *,<br class="gmail_msg">
             SmallVector<DwarfCompileUnit::GlobalExpr, 1>>><br class="gmail_msg">
<br class="gmail_msg">
so we can then simply iterate over the globals we found in each CU?<br class="gmail_msg">
<br class="gmail_msg">
I can't reliably establish a mapping from DICompileUnit -> DIGlobalVariable without scanning through each DICompileUnit's list of globals.</blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">Ah, that seems unfortunate - then we can't implicitly drop DIGlobalVariables when the GlobalVariable goes away, right? (means we /must/ have the DIGVE for the GV in the CUs list of DIVGEs - unlike subprograms, wehre we were able to remove that list and let DISubprograms drop implicitly when the llvm::Function went away)<br class="gmail_msg"><br class="gmail_msg">(Duncan's suggestion of having a cu link in the DIGV seems suitable - though, what we do if we then have some DIGVEs referring to one DIGV, and some referring to the other (how does this work for inlined functions today I wonder... ? *tries some things*))</div></div></div></blockquote><div><br>Yeah, this seems to be a problem - I think a problem, at least. Not what I would've expected.<br><br>inlined instances in one CU don't reference the same subprogram in another CU (I believe because the CU flag means the two DISubprograms aren't identical - but I guess we flag them as "distinct" anyway... hmm). <br><br>Duncan - thoughts? (maybe need to pull this conversation out, though - might be too much of a diversion here)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> A DIGlobalVariable has a scope field, but we don't enforce that that scope chain leads to a DICompileUnit. DIBuilder allows the creation of a DIGlobalVariable nested inside a composite type, for example (clang doesn't do this, but other frontends could).<br class="gmail_msg">
<br class="gmail_msg">
-- adrian<br class="gmail_msg">
<br class="gmail_msg">
> then you could do one lookup at the start of the cu loop, smaller lookups for the Second Loop (but probably still a second lookup for Processed - or this could be kept down in the Third Loop so we only needed to do it once per GV, instead of once per GVE in the CU), and the Third Loop could use no lookups at all, just loop over the GV->GVE mapping)<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
> <a href="https://reviews.llvm.org/D26769" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D26769</a><br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
<br class="gmail_msg">
_______________________________________________<br class="gmail_msg">
llvm-commits mailing list<br class="gmail_msg">
<a href="mailto:llvm-commits@lists.llvm.org" class="gmail_msg" target="_blank">llvm-commits@lists.llvm.org</a><br class="gmail_msg">
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" class="gmail_msg" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br class="gmail_msg">
</blockquote></div></div></blockquote></div></div>