[PATCH] D26769: [IR] Remove the DIExpression field from DIGlobalVariable.

[David Blaikie via llvm-commits]

On Thu, Dec 15, 2016 at 10:16 AM David Blaikie <dblaikie at gmail.com> wrote:

> On Thu, Dec 15, 2016 at 10:05 AM Adrian Prantl via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>
> > On Dec 14, 2016, at 9:10 PM, David Blaikie via Phabricator <
> reviews at reviews.llvm.org> wrote:
> >
> > dblaikie added inline comments.
> >
> >
> > ================
> > Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:500
> > +      GVMap[GVE->getVariable()].push_back({nullptr,
> GVE->getExpression()});
> > +    DenseSet<DIGlobalVariable *> Processed;
> > +    for (auto *GVE : CUNode->getGlobalVariables()) {
> > ----------------
> > (terminology:
> > First loop: the "for globals" loop outside the CU loop. This one
> finds/associates GVEs on actual globals.
> > 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)
> > Third loop: the second "for global variables" loop inside the CU loop -
> this one creates the global variables.
> >
> > Should this be scoped outside of the CU loop?
> >
> > (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?)
> >
> > 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)
> >
> > 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?)
> >
> > 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.
> >
> > There do seem like an awful lot of lookups in many directions in these
> three loops. Hrm. :/
> >
> > Maybe if we started off CU-specific? (the First Loop could create a map
> from CU -> GV -> GVEs,
>
> Are you thinking of a
>
>   DenseMap<DICompileUnit *,
>     DenseMap<DIGlobalVariable *,
>              SmallVector<DwarfCompileUnit::GlobalExpr, 1>>>
>
> so we can then simply iterate over the globals we found in each CU?
>
> I can't reliably establish a mapping from DICompileUnit ->
> DIGlobalVariable without scanning through each DICompileUnit's list of
> globals.
>
>
> 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)
>
> (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*))
>

Yeah, this seems to be a problem - I think a problem, at least. Not what I
would've expected.

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).

Duncan - thoughts? (maybe need to pull this conversation out, though -
might be too much of a diversion here)


>
>
> 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).
>
> -- adrian
>
> > 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)
>
>
>
> >
> >
> > https://reviews.llvm.org/D26769
> >
> >
> >
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161215/8f710317/attachment.html>