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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl marked 8 inline comments as done.
aprantl added inline comments.


================
Comment at: include/llvm/IR/GlobalVariable.h:177
+  using DIGlobalVarExpr =
+      PointerUnion<DIGlobalVariable *, DIGlobalVariableExpression *>;
+  /// Attach a DIGlobalVariable or DIGlobalVariableExpression.
----------------
dblaikie wrote:
> 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.
The union (the canonical definition is in DebugInfoMetadata.h exists so we don't need to waste a DIGlobalVariableExpression node on global variables that don't need an DIExpression. This is an optimization that Duncan suggested (though I can't find where at the moment).


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2209
           }
+          // FIXME: emitDebugInfoForGlobal() doesn't handle DIExpressions.
           emitDebugInfoForGlobal(G, GV, Asm->getSymbol(GV));
----------------
dblaikie wrote:
> Not quite sure what this means/what the implications are?
It's just something I noticed while updating the code. Could be a separate commit.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:191
+  if (Loc) {
+    DwarfExpr->finalize();
     addBlock(*VariableDIE, dwarf::DW_AT_location, Loc);
----------------
dblaikie wrote:
> Should finalize return the DIELoc instead of having it kept as a separate/parallel variable that DIEDwarfExpression is updating?
Not sure I understand. You mean like
```
class DIEDwarfExpression {
...
  DIELoc finalize() {
    DwarfExpression::finalize();
    return DIE;
  }
}

...
addBlock(*VariableDIE, dwarf::DW_AT_location, DwarfExpr.finalize());
```


================
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});
----------------
dblaikie wrote:
> (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)
Yeah not sure. Doing it this way saves a couple of pointers for global variables without expressions and makes the IR easier to read.


================
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:
> 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?
whoa. Thanks!


https://reviews.llvm.org/D26769