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

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

> On 2016-Dec-09, at 14:01, Adrian Prantl via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> 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).

(Might have been on the whiteboard.)

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