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

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-Dec-15, at 10:05, Adrian Prantl <aprantl at apple.com> 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. 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).

Why not just add a 'unit:' field, like we did for DISubprogram?

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