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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/IR/DebugInfoMetadata.h:1047-1048
+    }
+    bool operator==(const iterator &X) const { return I == X.I; }
+    bool operator!=(const iterator &X) const { return I != X.I; }
+  };
----------------
Usually best to make operators non-members where possible (helps with symmetric implicit conversions of LHS and RHS operands, for example). They can still be defined inline if you prefer - by declaring them as friends (which you probably want/need to do anyway)


================
Comment at: include/llvm/IR/GlobalVariable.h:177
+  using DIGlobalVarExpr =
+      PointerUnion<DIGlobalVariable *, DIGlobalVariableExpression *>;
+  /// Attach a DIGlobalVariable or DIGlobalVariableExpression.
----------------
Is this union for backwards compatibility? (so things can still refer directly to a DIGlobalVariable?) If so, might be nice to push that compatibility earlier - map old debug info by creating new DIGlobalVariableExpressions, rather than letting the old semantics remain further through the system.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:2972-2975
+        if (Expr)
+          Attach->addDebugInfo(DGVE);
+        else
+          Attach->addDebugInfo(DGV);
----------------
might be worth inverting this into a conditional operator

  if (Attach) {
    Attach->addDebugInfo(Expr ? DGVE : DGV);


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2209
           }
+          // FIXME: emitDebugInfoForGlobal() doesn't handle DIExpressions.
           emitDebugInfoForGlobal(G, GV, Asm->getSymbol(GV));
----------------
Not quite sure what this means/what the implications are?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:77
 DIE *DwarfCompileUnit::getOrCreateGlobalVariableDIE(
-    const DIGlobalVariable *GV, const GlobalVariable *Global) {
+    const DIGlobalVariable *GV, ArrayRef<GlobalExpr> Globals) {
   // Check for pre-existence.
----------------
Perhaps this should be called Exprs (since some of the expressions won't themselves be globals in LLVM's sense - some might just be direct constants, etc)?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:132
+  std::unique_ptr<DIEDwarfExpression> DwarfExpr;
+  for (auto &GE : Globals) {
+    const GlobalVariable *Global = GE.first;
----------------
  const auto &


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:191
+  if (Loc) {
+    DwarfExpr->finalize();
     addBlock(*VariableDIE, dwarf::DW_AT_location, Loc);
----------------
Should finalize return the DIELoc instead of having it kept as a separate/parallel variable that DIEDwarfExpression is updating?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:487-490
+      if (auto *GVE = GVU.dyn_cast<DIGlobalVariableExpression *>())
+        GVMap[GVE->getVariable()].push_back({&Global, GVE->getExpression()});
+      else 
+        GVMap[GVU.get<DIGlobalVariable *>()].push_back({&Global, nullptr});
----------------
(reinforcing previous comment - would be good to do a full migration rather than carrying more variety in the IR format - or at least map to the new semantics at a lower level (at bitcode/IR reading time) rather than having all the code walking a this IR having to conditionalize like this)


================
Comment at: lib/IR/DIBuilder.cpp:86-87
+  if (!AllGVs.empty()) {
+    std::vector<Metadata *> Elts;
+    Elts.resize(AllGVs.size());
+    for (auto GV : AllGVs)
----------------
  std::vector<Metadata*> Elts(AllGVs.size());


================
Comment at: lib/IR/DIBuilder.cpp:86-89
+    std::vector<Metadata *> Elts;
+    Elts.resize(AllGVs.size());
+    for (auto GV : AllGVs)
+      Elts.push_back(UnwrappedDIGlobalVarExpr(GV));
----------------
dblaikie wrote:
>   std::vector<Metadata*> Elts(AllGVs.size());
Oh - this might not be doing what you want (got test coverage?)

resize + push_back, means you'll have AllGVs.size() null pointers, then all the unwrapped elements after that.

Perhaps you meant reserve?


================
Comment at: lib/IR/DebugInfo.cpp:57
+    for (auto DIG : CU->getGlobalVariables()) {
       if (addGlobalVariable(DIG)) {
+        auto *GV = UnwrappedDIGlobalVarExpr(DIG).getVariable();
----------------
early return to reduce indentation?


https://reviews.llvm.org/D26769