[PATCH] D28005: Add a unit field to DIGlobalVariable, mirroring the design of DISubprogram.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: docs/SourceLevelDebugging.rst:456-458
+2. *Constant definitions* are reachable via the DICompileUnit's list of globals
+   which points to a DIGlobalVariableExpression with a DIGlobalVariable without
+   a unit field and a DIExpression describing a constant value.
----------------
aprantl wrote:
> dblaikie wrote:
> > Is this true? Do we go and change the DIGlobalVariable if we collapse an llvm::GlobalValue into a constant? So that it doesn't have a unit field anymore? I assume we don't do that, so some constants might point to DIGlobalVariables with a non-null unit field.
> Good catch, I should implement this.
Does this happen now? Where should I look for the code and test case?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:510-515
+  using GlobalExpr = DwarfCompileUnit::GlobalExpr;
+  using ExprListType = SmallVector<GlobalExpr, 1>;
+  using GVMapType = DenseMap<DIGlobalVariable *, std::unique_ptr<ExprListType>>;
+  using WorklistType =
+      std::vector<std::pair<DIGlobalVariable *, ExprListType *>>;
+  DenseMap<DICompileUnit *, std::pair<GVMapType, WorklistType>> UnitGVs;
----------------
This is a rather deeply nested data structure and I'm not sure the type aliases do enough to help improve readability. I wonder if it'd be worth writing a small class with named functions, some structs (rather than several std::pairs), etc. At least some more detailed commentary about the structure and invariants of this data structure. (eg: I'm a bit confused by how the contents of the GVMapType and WorklistType relate, or don't)

For example the GVMapType and WorklistType probably deserve a type rather than only std::pair - since insertIntoWorklist uses both seems they're sort of connected-ish.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:513
+  using GVMapType = DenseMap<DIGlobalVariable *, std::unique_ptr<ExprListType>>;
+  using WorklistType =
+      std::vector<std::pair<DIGlobalVariable *, ExprListType *>>;
----------------
Not sure if this is the best terminology here. At least my thought when I see "worklist" is an algorithm that starts with a list of things to process and the act of processing those things can add new things to the worklist. That doesn't seem to be the case here


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:547
+    // Collect DIGlobalVariables reachable via the DICompileUnit.
+    auto &GVMapWorklist = UnitGVs[CUNode];
+    GVMapType &GVMap = GVMapWorklist.first;
----------------
Would be marginally more efficient not to add an element to UnitGVs that will never be used/retrieved/queried. Maybe not worth the added verbosity, though.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:554
+      ExprListType &Exprs = insertIntoWorklist(GVMap, Worklist, Var);
+      if (std::none_of(Exprs.begin(), Exprs.end(),
+                       [&Expr](GlobalExpr &GE) { return GE.Expr == Expr; }))
----------------
llvm::none_of?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:555
+      if (std::none_of(Exprs.begin(), Exprs.end(),
+                       [&Expr](GlobalExpr &GE) { return GE.Expr == Expr; }))
+        Exprs.push_back({nullptr, Expr});
----------------
I'd generally suggest using [&] rather than explicit captures if the lambda doesn't outlive its scope.


https://reviews.llvm.org/D28005