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

[Duncan P. N. Exon Smith]

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

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

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