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

[David Blaikie via Phabricator via llvm-commits]

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