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

[David Blaikie via llvm-commits]

On Fri, Dec 9, 2016 at 2:13 PM Adrian Prantl <aprantl at apple.com> wrote:

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

Sort of. My answer wasn't really satisfactory/accurate - your explanation
refreshed my understanding so I'll make a stronger statement:

We don't have infrastructure for emitting function declarations (except as
class members), and I would expect global variables to be treated the same.
Which is to say I'm not sure we need infrastructure for global variables
that have no value.

Ah, I see - you mean the case where it does have a location, it's just not
described by a complex expression (it's just the address itself). Is it
worth optimizing for? We don't do that for local variables, right?

- Dave


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