[PATCH] DebugInfo: Remove 'inlinedAt:' field from MDLocalVariable/DIVariable

[David Blaikie]

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.


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


>
> 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`.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150402/974dec6f/attachment.html>