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

[Adrian Prantl via llvm-commits]

> On Dec 9, 2016, at 2:08 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Fri, Dec 9, 2016 at 2:01 PM 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).
> 
> I assume the plan is to treat global variables the same way we treat functions - drop them if we no longer have a definition. (potentially keep a list of any that get collapsed to a constant/no asociation from an llvm::GlobalValue - but, say, if we did LTO and just optimized away a global variable because it was unused, we'd drop it the same as we drop functions). So it seems strange to me to optimize that case that I think we don't seem to be/intend to care about/have happen much/at all?

Maybe we are talking about different things. The PointerUnion allows us to describe a global variable that doesn't need a DIExpression like so:

  @g = global i32 0, !dbg !0
  !0 = DIGlobalVariable(name: "g", ...)

without it, we'd have to describe it like so:

  @g = global i32 0, !dbg !0
  !0 = DIGlobalVariableExpression(var: !1, expr: null)
  !1 = DIGlobalVariable(name: "g", ...)

And I think this decision is unaffected by how we handle global variables in optimizations. Let me know if I'm missing the point.

-- adrian

>  
> 
> 
> ================
> 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.
> 
> Might be good - just to discuss.
>  
> ================
> 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());
> 
> Yep, something like that. (presumably a DILoc*, but yeah - that idea)