[PATCH] DebugInfo: Remove 'inlinedAt:' field from MDLocalVariable/DIVariable
David Blaikie
dblaikie at gmail.com
Thu Apr 2 13:28:47 PDT 2015
On Apr 2, 2015 1:05 PM, "Duncan P. N. Exon Smith" <dexonsmith at apple.com>
wrote:
>
>
> > On 2015-Apr-02, at 12:03, Duncan P. N. Exon Smith <dexonsmith at apple.com>
wrote:
> >
> >>
> >> On 2015-Apr-02, at 09:37, David Blaikie <dblaikie at gmail.com> wrote:
> >>
> >>
> >>
> >> On Thu, Apr 2, 2015 at 9:32 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
> >>
> >>> On 2015-Apr-02, at 09:14, David Blaikie <dblaikie at gmail.com> wrote:
> >>>
> >>>
> >>>
> >>> On Thu, Apr 2, 2015 at 9:10 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
> >>>
> >>>> On 2015-Apr-02, at 08:01, David Blaikie <dblaikie at gmail.com> wrote:
> >>>>
> >>>>
> >>>> On Apr 1, 2015 11:36 PM, "Duncan P. N. Exon Smith" <
dexonsmith at apple.com> wrote:
> >>>>>
> >>>>> When I was gathering LTO heap profiles a few weeks ago, the single
> >>>>> largest `Metadata` contributer to memory usage was
`MDLocalVariable`,
> >>>>> with a huge chunk coming from `MDLocalVariable::withInline()`.
> >>>>>
> >>>>> This patch stops storing where an `MDLocalVariable` has been
inlined.
> >>>>> This inlined-at info was used two ways:
> >>>>>
> >>>>> - To tell the backend where a variable was inlined.
> >>>>> - To create a unique id for each inlined variable.
> >>>>>
> >>>>> However, every instruction has a `!dbg` attachment with the same
> >>>>> information in its inlined-at field. AFAICT, David Blaikie's work
to
> >>>>> make that accurate has paid off, since this naive patch (which just
> >>>>> removes the field entirely) passes all the tests.
> >>>>>
> >>>>> The unique id for inlined variables is now a typedef called
> >>>>> `InlinedVariable` of `std::pair<MDLocalVariable*, MDLocation*>`.
> >>>>>
> >>>>> Besides the patch, I've attached the upgrade script I used to update
> >>>>> testcases.
> >>>>>
> >>>>> Any reason not to commit? (Am I missing something here?)
> >>>>
> >>>> Last time I tried this (not sure whether I got as far as
commuting/referring it) either I hit those issues I have since fixed (maybe
that's part of what me going on that kick) or some other issues...
> >>>>
> >>>> In any case, would it be possible to run some tests that assert that
these two locations are the same (possibly this assertion could take the
place of the construction of these excessive/redundant/bloated inlined
DIVariables - so we could gain the memory savings while still leaving the
checking in just in case there are new/weird/interesting cases) for now?
> >>>
> >>> I can't remove the field and assert about its value at the same time
;).
> >>>
> >>> Sorry, what I meant (& perhaps it's still not possible) is to assert,
at the point where the field would be initialized/the extra DIVariable
created, instead just assert that the inlinedAt location we were about to
use is the same as the dbgloc we would've used.
> >>>
> >>
> >> As it happens, that's already true by construction. The only calls to
> >> `createInlinedVariable()` (which is the only caller of
> >> `MDLocalVariable::withInline()`) are here in `InlineFunction.cpp`:
> >>
> >> lib/Transforms/Utils/InlineFunction.cpp-905- } else {
> >> lib/Transforms/Utils/InlineFunction.cpp-906-
BI->setDebugLoc(updateInlinedAtInfo(DL, InlinedAtNode, BI->getContext(),
IANodes));
> >> lib/Transforms/Utils/InlineFunction.cpp-907- if (DbgValueInst
*DVI = dyn_cast<DbgValueInst>(BI)) {
> >> lib/Transforms/Utils/InlineFunction.cpp-908- LLVMContext &Ctx
= BI->getContext();
> >> lib/Transforms/Utils/InlineFunction.cpp-909- MDNode
*InlinedAt = BI->getDebugLoc().getInlinedAt();
> >> lib/Transforms/Utils/InlineFunction.cpp-910-
DVI->setOperand(2, MetadataAsValue::get(
> >> lib/Transforms/Utils/InlineFunction.cpp:911:
Ctx, createInlinedVariable(DVI->getVariable(),
> >> lib/Transforms/Utils/InlineFunction.cpp-912-
InlinedAt, Ctx)));
> >> lib/Transforms/Utils/InlineFunction.cpp-913- } else if
(DbgDeclareInst *DDI = dyn_cast<DbgDeclareInst>(BI)) {
> >> lib/Transforms/Utils/InlineFunction.cpp-914- LLVMContext &Ctx
= BI->getContext();
> >> lib/Transforms/Utils/InlineFunction.cpp-915- MDNode
*InlinedAt = BI->getDebugLoc().getInlinedAt();
> >> lib/Transforms/Utils/InlineFunction.cpp-916-
DDI->setOperand(1, MetadataAsValue::get(
> >> lib/Transforms/Utils/InlineFunction.cpp:917:
Ctx, createInlinedVariable(DDI->getVariable(),
> >> lib/Transforms/Utils/InlineFunction.cpp-918-
InlinedAt, Ctx)));
> >> lib/Transforms/Utils/InlineFunction.cpp-919- }
> >> lib/Transforms/Utils/InlineFunction.cpp-920- }
> >>
> >> However, the `assert()`s in my patch are firing so I think we're
> >> losing the inlined-at somewhere in the backend before we construct the
> >> variables. (The Verifier checks fail on a few testcases too, but it
> >> looks like that's just bitrot.)
> >>
> >> Alrighty - be curious to see where. Totally happy to help out with
fixing/debugging/reducing/whatever these issues.
> >
> > 0003 (attached) fixes a bug in RAFast::spillVirtReg() and adds a ton of
> > new assertions to find root cause. The only remaining failing test is
> > the aptly named:
> >
> > test/DebugInfo/incorrect-variable-debugloc.ll
> >
> > This is the assertion that fires:
> >
> > Assertion failed: (VarIA == LocIA && "Expected inlined-at fields to
agree"), function EmitFuncArgumentDbgValue, file
/Users/dexonsmith/data/llvm/debug-info/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp,
line 4512.
> >
> > Local backtrace:
> > * frame #5: 0x0000000101a2c1f3
llc`llvm::SelectionDAGBuilder::EmitFuncArgumentDbgValue(this=0x0000000104327c70,
V=0x000000010430f480, Variable=0x000000010431db78, Expr=0x00000001043104c0,
Offset=0, IsIndirect=false, N=0x00007fff5fbfcd58) + 1459 at
SelectionDAGBuilder.cpp:4512
> > frame #6: 0x0000000101a2ba20
llc`llvm::SelectionDAGBuilder::resolveDanglingDebugInfo(this=0x0000000104327c70,
V=0x000000010430f480, Val=SDValue at 0x00007fff5fbfcd58) + 304 at
SelectionDAGBuilder.cpp:1008
> > frame #7: 0x0000000101a2c6a7
llc`llvm::SelectionDAGBuilder::getCopyFromRegs(this=0x0000000104327c70,
V=0x000000010430f480, Ty=0x00000001048156f0) + 567 at
SelectionDAGBuilder.cpp:1032
> > frame #8: 0x0000000101a2d690
llc`llvm::SelectionDAGBuilder::getValue(this=0x0000000104327c70,
V=0x000000010430f480) + 128 at SelectionDAGBuilder.cpp:1048
> > frame #9: 0x0000000101a242f3
llc`llvm::SelectionDAGBuilder::visitGetElementPtr(this=0x0000000104327c70,
I=0x0000000104312590) + 99 at SelectionDAGBuilder.cpp:3383
> >
> > Looks like `resolveDanglingDebugInfo()` doesn't set the DebugLoc
> > correctly?
>
> I met with Adrian offline: it looks like `getCurDebugLoc()` will be
> the wrong thing in `SelectionDAGBuilder::EmitFuncArgumentDbgValue()`
> and we should just be passing the correct debug loc through.
Seems likely. I've hit one or two of those.
>
> >
> >
<0001-Verifier-Check-that-inlined-at-locations-agree.patch><0002-CodeGen-Fix-MachineInstr-print-for-DBG_VALUE.patch><0003-WIP-Add-assertions-at-MI-creation-time-for-valid-inl.patch>
> >>
> >>
> >>> I'll start by adding the assertion, and after a while (a day? a week?)
> >>> if no bot complains I'll rip out the field. (If bots do complain we
can
> >>> fix bugs until they're happy.)
> >>>
> >>> *nod* Sounds good - I'm not too fussed about waiting ages, but at
least a good clang/opt bootstrap, etc. A week's probably not unreasonable.
> >>>
> >>> Currently running `ninja check-all` on the attached patch; I'll commit
> >>> it after fixing up any failing testcases and we can go from there.
> >>>
> >>> Looks reasonable (though how'd you pick those 3-4 places to add
assertions? Are they all the places that use DIVariable::getInlinedAt?)
> >>
> >> These are the spots that my first patch constructed an
`InlinedVariable`
> >> union the callers of `MDLocalVariable::getInlinedAt()` that have a
> >> `!dbg` attachment around.
> >>
> >> I can't quite parse that sentence... ^
> >>
> >
> > (places my first patch constructed an `InlinedVariable`)
> > union
> > (places anyone calls `MDLocalVariable::getInlinedAt()` and has a `!dbg`)
> >
> >
> >
> >>
> >> There are a couple of callers to `MDLocalVariable::getInlinedAt()` that
> >> don't have access to the `!dbg` attachment until my first patch passes
> >> them through via `InlinedVariable`.
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150402/0abd50e7/attachment.html>
More information about the llvm-commits
mailing list