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

[David Blaikie]

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.


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