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

[David Blaikie via llvm-commits]

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?


>
>
> ================
> 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)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161209/d5424583/attachment.html>