llvm dev - Apr 2016 - [RFC] Lazy-loading of debug info metadata

On Fri, Apr 15, 2016 at 2:27 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
> > On 2016-Apr-15, at 10:27, David Blaikie <dblaikie at gmail.com>
wrote:
> >
> >
> >
> > On Fri, Apr 15, 2016 at 9:50 AM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > Since I haven't heard any objections to the direction, I'm
planning to
> > commit this (step 4) when I find some cycles; likely over the weekend.
> >
> > To make this more concrete (in case someone is silently concerned)
I've
> > posted my WIP patches below.  They apply cleanly to r266414.  There
are
> > a few mechanical changes missing that are tracked in the commit
> > messages (such as LangRef, or new tests in some cases).  To get
> > `ninja check` to pass after the final commit you'll need to run
the
> > attached upgrade script.  The clang changes are truly uninteresting so
> > I've left them out.
> >
> >   - 0001/2: Prep commits I need to flush out (sorry for the noise).
> >   - 0003: Add an explicit type map for ODR type uniquing.
> >   - 0004: Prep.
> >   - 0005: Add ODR type uniquing of members.
> >
> > What's this ^ for?
> >
> > In theory, the list of /members/ of a type shouldn't vary between
> instances. The special cases (implicit special members, member function
> template specializations, and nested types) don't appear in the member
list
> - instead they just list the type as their scope (so it's one
directional,
> instead of bidirectional)
> >
> > Is this to handle ODR violations? We currently don't handle them,
right
> - we just pick one instance of the type & the user gets to keep the
pieces.
> Is there a particular motivation for changing that? Some place where
it's
> more problematic, we have to support it, etc?
>
> This is a strange one; I probably should have called it out actually.
> It's a bugfix vs. ToT; and without it, 0006 regresses DWARF output.
>
> File and line can change without violating ODR, which means that we
> end up with duplicate members.  I think I may have written something
> up in the WIP commit message for 0005, but also have a look in tree at
> test/Linker/type-unique-odr-a.ll.
>
> It's legal, e.g., to have the following in two separate files, as long
> as the sequence of tokens is identical:
> --
> struct S { void foo(); };
> --
> However, the DISubprogram(isDefinition: false) for S::foo will not be
> structurally equivalent (since the 'file:' node won't match),
so we
> currently end up with two metadata nodes.  The same thing happens for
> the DICompositeType for `S` (and also applies to any fields).
>
> The checked-in testcase confirms that the description of `S` only has
> one copy of `S::foo` in the DWARF output.  (The testcase doesn't
provide
> good enough coverage actually; it gets lucky because the definition of
> S::foo is in the second file being linked (type-unique-odr-b.ll).  If
> you reverse the order of the files, then `S` will have two copies of
> `S::foo`.)
>
> 0005 solves the problem up front (and in both directions; I'll add a
RUN
> line to test/Linker/type-unique-odr-a.ll).
>
> I came across this because the testcase started failing with 0006.  The
> current (but not-quite-sufficient) logic relies on the resolve() logic
> in DwarfDebug.cpp, which indirects through the merged
'retainedTypes:'
> map (the asymmetry in building the merged map explains why the in-tree
> logic only works for one linking direction).
>
Not sure I'm quite following.

I think I understand the circumstance you're describing (indeed, the type
itself can be defined on different lines/files without violating the ODR,
or its members, or, etc... ). But if the subprogram declaration is in a
different file in one definition - that'd make the composite type itself
different (metadata uniquing - composite type references the subprogram,
etc).

But we should only every find one version of the type (all references to
the type should be via string-based dityperefs). Sure, we'll pick one at
random, but when/how would we get two?

>
> >
> >   - 0006: Remove DITypeRef (string-based references) and strip down
> >     'retainedTypes:'.
> >
> > Once this is done, I expect the bitcode block layout improvements for
> > lazy-loading of subprograms and composite types (steps 3 and 5 from
> > this RFC) to be fairly straightforward.
> >
> >
> >
> >
> > > On 2016-Mar-29, at 19:11, Eric Christopher <echristo at
gmail.com> wrote:
> > >
> > > I have no objections to any of this FWIW :)
> > >
> > > -eric
> > >
> > > On Tue, Mar 29, 2016 at 6:46 PM Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > >
> > > > On 2016-Mar-22, at 20:11, Eric Christopher <echristo at
gmail.com>
> wrote:
> > > >
> > > >> On Tue, Mar 22, 2016 at 8:04 PM David Blaikie
<dblaikie at gmail.com>
> wrote:
> > > >> +pcc, who had some other ideas/patch out for improving
memory usage
> of debug info
> > > >> +Reid, who's responsible for the
windows/CodeView/PDB debug info
> which is motivating some of the ideas about changes to type emission
> > > >
> > > > So I discussed this with Adrian and Mehdi at the social last
> Thursday and I'm getting set to finish the write up. I think it'll
have
> some bearing on this proposal as I think it'll change how we want to
take a
> look at the format of DISubprogram metadata a bit more.
> > >
> > > (The interesting bit here is to make a clearer split between
> > > DISubprogram declarations (part of the type hierarchY) and
> > > DISubprogram definitions (part of the code/line table/variable
> > > locations).  I think that'll end up being mostly orthogonal
to what
> > > I'm trying to do.)
> > >
> > > > That said, most of it is orthogonal to the changes Duncan is
talking
> about here. Just puts the pressure on to get the other proposal written up.
> > >
> > > Which is now here:
> > > http://lists.llvm.org/pipermail/llvm-dev/2016-March/097773.html
> > >
> > > >> Baking into the IR more about types as units has pretty
direct
> overlap with Reid/CodeView/etc - so, yeah, that'll takes ome discussion
> (but, as you say, it's not in your immediate plan anyway, so we can
come
> back to that - but would be good for whoever gets there first to discuss it
> with the others)
> > >
> > > After thinking for a few days, I don't think this will bake
anything
> > > new into the IR.  If anything it removes a few special cases.
> > >
> > > More at the bottom.
> > >
> > > >>> Motivation
> > > >>> =========> > > >>>
> > > >>> Based on some analysis Mehdi ran (ping him for
details), there are
> three
> > > >>> (related) compile-time bottlenecks we're seeing
with `-flto=thin
> -g`:
> > > >>>
> > > >>>  a) Reading the large number of Metadata bitcode
records in the
> global
> > > >>>     metadata block.  I'm talking about raw
`BitStreamer` calls
> here.
> > > >>>
> > > >>>  b) Creating unnecessary `DI*` instances (that
aren't relevant to
> code).
> > > >>>
> > > >>>  c) Emitting unnecessary `DI*` instances (that
aren't relevant to
> code).
> > > >>>
> > > >>> Here is my recollection of some peak memory stats on
a small
> testcase
> > > >>> during thin-LTO, which should be a decent indicator
of (b):
> > > >>>
> > > >>>   - ~150MB: DILocation
> > > >>>   - ~100MB: DISubprogram
> > > >>>   - ~70MB: DILocalVariable
> > > >>>   - ~50MB: (cumulative) DIType descendents
> > > >>>
> > > >>> It looks, suprisingly, like types are not the
primary bottleneck.
> > >
> > > (Probably wrong for (a), BTW.  Caveats matter.)
> > >
> > > >>> There are caveats:
> > > >>>
> > > >>>   - `DISubprogram` declarations -- member function
descriptors --
> are
> > > >>>     part of the type hierarchy.
> > > >>>   - Most of the type hierarchy gets uniqued at parse
time.
> > > >>>   - As a result, these data are a poor indicator for
(a).
> > >
> > > ((a) is the main bottleneck for compile-time of -flto=thin (since
it's
> > > quadratic in the number of files).  (b) only affects memory. 
Also
> > > important, but at least it scales linearly.)
> > >
> > > >>> Even so, non-types are substantial.
> > > >>>
> > > >>> Proposal
> > > >>> =======> > > >>>
> > > >>> Short version
> > > >>> -------------
> > > >>>
> > > >>>  4. Remove `DICompositeType`s from `retainedTypes:`,
similar to
> (2).
> > >
> > > This is the part that's relevant to the new RFC Eric just
posted.
> > >
> > > >>> Long version
> > > >>> -------------
> > > >>>
> > > >>>  4. Implement my proposal to remove the
`DICompositeType` name map
> from
> > > >>>     `retainedTypes:`.
> > > >>>
> > > >>>
>
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160125/327936.html
> > > >>>
> > > >>>     Similar to (2) above, this will naturally filter
the types
> that get
> > > >>>     linked in to the ones actually used by the code
being linked.
> > > >>>
> > > >>>     It should also allow the reader to skip records
for types that
> have
> > > >>>     already been loaded in the main module.
> > >
> > > The essential things I want to accomplish with this part:
> > >
> > >   - Make `type:` operands less special: instead of referencing
types
> > >     indirectly through MDString, point directly at the type node.
> > >
> > >   - Stop using `retainedTypes:` by default (only for -gfull,
etc.).
> > >
> > >   - Avoid blowing up memory in -flto=full (which converting
MDString
> > >     references back to pointers would do naively, through
> > >     re-introducing cycles).  Note that this needs to be handled
> > >     somehow at BitcodeReader time.
> > >
> > > After chatting with Eric, I don't think this conflicts at all
with the
> > > other RFC.  Unifying the `type:` operands might actually help
both.
> > >
> > > One good point David mentioned last week was that we don't
want to
> > > teach the IR any more about types.  Rather than inventing some
new
> > > context (as I suggested originally), I figure LTO plugins can
just
> > > pass a (StringRef -> DIType*) map to the BitcodeReader, and
the Module
> > > itself doesn't need to know anything about it.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160415/c74de578/attachment.html>

> On 2016-Apr-15, at 14:53, David Blaikie <dblaikie at gmail.com>
wrote:
> 
> 
> 
>> On Fri, Apr 15, 2016 at 2:27 PM, Duncan P. N. Exon Smith <dexonsmith
at apple.com> wrote:
>> 
>> > On 2016-Apr-15, at 10:27, David Blaikie <dblaikie at
gmail.com> wrote:
>> >
>> >
>> >
>> > On Fri, Apr 15, 2016 at 9:50 AM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>> > Since I haven't heard any objections to the direction, I'm
planning to
>> > commit this (step 4) when I find some cycles; likely over the
weekend.
>> >
>> > To make this more concrete (in case someone is silently concerned)
I've
>> > posted my WIP patches below.  They apply cleanly to r266414. 
There are
>> > a few mechanical changes missing that are tracked in the commit
>> > messages (such as LangRef, or new tests in some cases).  To get
>> > `ninja check` to pass after the final commit you'll need to
run the
>> > attached upgrade script.  The clang changes are truly
uninteresting so
>> > I've left them out.
>> >
>> >   - 0001/2: Prep commits I need to flush out (sorry for the
noise).
>> >   - 0003: Add an explicit type map for ODR type uniquing.
>> >   - 0004: Prep.
>> >   - 0005: Add ODR type uniquing of members.
>> >
>> > What's this ^ for?
>> >
>> > In theory, the list of /members/ of a type shouldn't vary
between instances. The special cases (implicit special members, member function
template specializations, and nested types) don't appear in the member list
- instead they just list the type as their scope (so it's one directional,
instead of bidirectional)
>> >
>> > Is this to handle ODR violations? We currently don't handle
them, right - we just pick one instance of the type & the user gets to keep
the pieces. Is there a particular motivation for changing that? Some place where
it's more problematic, we have to support it, etc?
>> 
>> This is a strange one; I probably should have called it out actually.
>> It's a bugfix vs. ToT; and without it, 0006 regresses DWARF output.
>> 
>> File and line can change without violating ODR, which means that we
>> end up with duplicate members.  I think I may have written something
>> up in the WIP commit message for 0005, but also have a look in tree at
>> test/Linker/type-unique-odr-a.ll.
>> 
>> It's legal, e.g., to have the following in two separate files, as
long
>> as the sequence of tokens is identical:
>> --
>> struct S { void foo(); };
>> --
>> However, the DISubprogram(isDefinition: false) for S::foo will not be
>> structurally equivalent (since the 'file:' node won't
match), so we
>> currently end up with two metadata nodes.  The same thing happens for
>> the DICompositeType for `S` (and also applies to any fields).
>> 
>> The checked-in testcase confirms that the description of `S` only has
>> one copy of `S::foo` in the DWARF output.  (The testcase doesn't
provide
>> good enough coverage actually; it gets lucky because the definition of
>> S::foo is in the second file being linked (type-unique-odr-b.ll).  If
>> you reverse the order of the files, then `S` will have two copies of
>> `S::foo`.)
>> 
>> 0005 solves the problem up front (and in both directions; I'll add
a RUN
>> line to test/Linker/type-unique-odr-a.ll).
>> 
>> I came across this because the testcase started failing with 0006.  The
>> current (but not-quite-sufficient) logic relies on the resolve() logic
>> in DwarfDebug.cpp, which indirects through the merged
'retainedTypes:'
>> map (the asymmetry in building the merged map explains why the in-tree
>> logic only works for one linking direction).
> 
> Not sure I'm quite following.
> 
> I think I understand the circumstance you're describing (indeed, the
type itself can be defined on different lines/files without violating the ODR,
or its members, or, etc... ). But if the subprogram declaration is in a
different file in one definition - that'd make the composite type itself
different (metadata uniquing - composite type references the subprogram, etc).
> 
> But we should only every find one version of the type (all references to
the type should be via string-based dityperefs). Sure, we'll pick one at
random, but when/how would we get two?
We get two because one of the modules contributes a definition of S::foo,
and it may be pointing at the declaration from a version of S that isn't
blessed.  If you reverse the RUN line in the in-tree testcase you can see
what happens, but here's a quick hand-written version:
--
!llvm.dbg.cus = !{!0, !4}

!0 = distinct !DICompileUnit(file: !1, retainedTypes: !{!2})
!1 = !DIFile(filename: "a.cpp")
!2 = !DICompositeType(file: !1, name: "S", elements: !{!3},
      identifier: "_ZTS1S")
!3 = !DISubprogram(isDefinition: false, file: !1, name: "foo",
      scope: !2, linkageName: "_ZN1S3fooEv")

!4 = distinct !DICompileUnit(file: !5, retainedTypes: !{!6})
!5 = !DIFile(filename: "b.cpp")
!6 = !DICompositeType(file: !5, name: "S", elements: !{!7},
      identifier: "_ZTS1S")
!7 = !DISubprogram(isDefinition: false, file: !5, name: "foo",
      scope: !6, linkageName: "_ZN1S3fooEv")

define void @foo() !dbg !8 {
  ret void
}

!8 = !DISubprogram(isDefinition: true, file: !5, name: "foo",
      unit: !4, linkageName: "_Z3foov", declaration: !7)
--

If DwarfDebug picks !6 for the canonical "_ZTS1S", then there's no
problem.  !6 points at its function !7, and the definition !8 also
points at !7.

On the other hand, if DwarfDebug picks !2 for "_ZTS1S", then !2 will
pull in !3 and !8 will still pull in !7.  The llvm-dwarfdebug output
will emit both !3 and !7 inside of !2.
> 
> >
> >   - 0006: Remove DITypeRef (string-based references) and strip down
> >     'retainedTypes:'.
> >
> > Once this is done, I expect the bitcode block layout improvements for
> > lazy-loading of subprograms and composite types (steps 3 and 5 from
> > this RFC) to be fairly straightforward.
> >
> >
> >
> >
> > > On 2016-Mar-29, at 19:11, Eric Christopher <echristo at
gmail.com> wrote:
> > >
> > > I have no objections to any of this FWIW :)
> > >
> > > -eric
> > >
> > > On Tue, Mar 29, 2016 at 6:46 PM Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
> > >
> > > > On 2016-Mar-22, at 20:11, Eric Christopher <echristo at
gmail.com> wrote:
> > > >
> > > >> On Tue, Mar 22, 2016 at 8:04 PM David Blaikie
<dblaikie at gmail.com> wrote:
> > > >> +pcc, who had some other ideas/patch out for improving
memory usage of debug info
> > > >> +Reid, who's responsible for the
windows/CodeView/PDB debug info which is motivating some of the ideas about
changes to type emission
> > > >
> > > > So I discussed this with Adrian and Mehdi at the social last
Thursday and I'm getting set to finish the write up. I think it'll have
some bearing on this proposal as I think it'll change how we want to take a
look at the format of DISubprogram metadata a bit more.
> > >
> > > (The interesting bit here is to make a clearer split between
> > > DISubprogram declarations (part of the type hierarchY) and
> > > DISubprogram definitions (part of the code/line table/variable
> > > locations).  I think that'll end up being mostly orthogonal
to what
> > > I'm trying to do.)
> > >
> > > > That said, most of it is orthogonal to the changes Duncan is
talking about here. Just puts the pressure on to get the other proposal written
up.
> > >
> > > Which is now here:
> > > http://lists.llvm.org/pipermail/llvm-dev/2016-March/097773.html
> > >
> > > >> Baking into the IR more about types as units has pretty
direct overlap with Reid/CodeView/etc - so, yeah, that'll takes ome
discussion (but, as you say, it's not in your immediate plan anyway, so we
can come back to that - but would be good for whoever gets there first to
discuss it with the others)
> > >
> > > After thinking for a few days, I don't think this will bake
anything
> > > new into the IR.  If anything it removes a few special cases.
> > >
> > > More at the bottom.
> > >
> > > >>> Motivation
> > > >>> =========> > > >>>
> > > >>> Based on some analysis Mehdi ran (ping him for
details), there are three
> > > >>> (related) compile-time bottlenecks we're seeing
with `-flto=thin -g`:
> > > >>>
> > > >>>  a) Reading the large number of Metadata bitcode
records in the global
> > > >>>     metadata block.  I'm talking about raw
`BitStreamer` calls here.
> > > >>>
> > > >>>  b) Creating unnecessary `DI*` instances (that
aren't relevant to code).
> > > >>>
> > > >>>  c) Emitting unnecessary `DI*` instances (that
aren't relevant to code).
> > > >>>
> > > >>> Here is my recollection of some peak memory stats on
a small testcase
> > > >>> during thin-LTO, which should be a decent indicator
of (b):
> > > >>>
> > > >>>   - ~150MB: DILocation
> > > >>>   - ~100MB: DISubprogram
> > > >>>   - ~70MB: DILocalVariable
> > > >>>   - ~50MB: (cumulative) DIType descendents
> > > >>>
> > > >>> It looks, suprisingly, like types are not the
primary bottleneck.
> > >
> > > (Probably wrong for (a), BTW.  Caveats matter.)
> > >
> > > >>> There are caveats:
> > > >>>
> > > >>>   - `DISubprogram` declarations -- member function
descriptors -- are
> > > >>>     part of the type hierarchy.
> > > >>>   - Most of the type hierarchy gets uniqued at parse
time.
> > > >>>   - As a result, these data are a poor indicator for
(a).
> > >
> > > ((a) is the main bottleneck for compile-time of -flto=thin (since
it's
> > > quadratic in the number of files).  (b) only affects memory. 
Also
> > > important, but at least it scales linearly.)
> > >
> > > >>> Even so, non-types are substantial.
> > > >>>
> > > >>> Proposal
> > > >>> =======> > > >>>
> > > >>> Short version
> > > >>> -------------
> > > >>>
> > > >>>  4. Remove `DICompositeType`s from `retainedTypes:`,
similar to (2).
> > >
> > > This is the part that's relevant to the new RFC Eric just
posted.
> > >
> > > >>> Long version
> > > >>> -------------
> > > >>>
> > > >>>  4. Implement my proposal to remove the
`DICompositeType` name map from
> > > >>>     `retainedTypes:`.
> > > >>>
> > > >>>    
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160125/327936.html
> > > >>>
> > > >>>     Similar to (2) above, this will naturally filter
the types that get
> > > >>>     linked in to the ones actually used by the code
being linked.
> > > >>>
> > > >>>     It should also allow the reader to skip records
for types that have
> > > >>>     already been loaded in the main module.
> > >
> > > The essential things I want to accomplish with this part:
> > >
> > >   - Make `type:` operands less special: instead of referencing
types
> > >     indirectly through MDString, point directly at the type node.
> > >
> > >   - Stop using `retainedTypes:` by default (only for -gfull,
etc.).
> > >
> > >   - Avoid blowing up memory in -flto=full (which converting
MDString
> > >     references back to pointers would do naively, through
> > >     re-introducing cycles).  Note that this needs to be handled
> > >     somehow at BitcodeReader time.
> > >
> > > After chatting with Eric, I don't think this conflicts at all
with the
> > > other RFC.  Unifying the `type:` operands might actually help
both.
> > >
> > > One good point David mentioned last week was that we don't
want to
> > > teach the IR any more about types.  Rather than inventing some
new
> > > context (as I suggested originally), I figure LTO plugins can
just
> > > pass a (StringRef -> DIType*) map to the BitcodeReader, and
the Module
> > > itself doesn't need to know anything about it.

On Fri, Apr 15, 2016 at 4:04 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
> > On 2016-Apr-15, at 14:53, David Blaikie <dblaikie at gmail.com>
wrote:
> >
> >
> >
> >> On Fri, Apr 15, 2016 at 2:27 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> >>
> >> > On 2016-Apr-15, at 10:27, David Blaikie <dblaikie at
gmail.com> wrote:
> >> >
> >> >
> >> >
> >> > On Fri, Apr 15, 2016 at 9:50 AM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> >> > Since I haven't heard any objections to the direction,
I'm planning to
> >> > commit this (step 4) when I find some cycles; likely over the
weekend.
> >> >
> >> > To make this more concrete (in case someone is silently
concerned)
> I've
> >> > posted my WIP patches below.  They apply cleanly to r266414. 
There
> are
> >> > a few mechanical changes missing that are tracked in the
commit
> >> > messages (such as LangRef, or new tests in some cases).  To
get
> >> > `ninja check` to pass after the final commit you'll need
to run the
> >> > attached upgrade script.  The clang changes are truly
uninteresting so
> >> > I've left them out.
> >> >
> >> >   - 0001/2: Prep commits I need to flush out (sorry for the
noise).
> >> >   - 0003: Add an explicit type map for ODR type uniquing.
> >> >   - 0004: Prep.
> >> >   - 0005: Add ODR type uniquing of members.
> >> >
> >> > What's this ^ for?
> >> >
> >> > In theory, the list of /members/ of a type shouldn't vary
between
> instances. The special cases (implicit special members, member function
> template specializations, and nested types) don't appear in the member
list
> - instead they just list the type as their scope (so it's one
directional,
> instead of bidirectional)
> >> >
> >> > Is this to handle ODR violations? We currently don't
handle them,
> right - we just pick one instance of the type & the user gets to keep
the
> pieces. Is there a particular motivation for changing that? Some place
> where it's more problematic, we have to support it, etc?
> >>
> >> This is a strange one; I probably should have called it out
actually.
> >> It's a bugfix vs. ToT; and without it, 0006 regresses DWARF
output.
> >>
> >> File and line can change without violating ODR, which means that
we
> >> end up with duplicate members.  I think I may have written
something
> >> up in the WIP commit message for 0005, but also have a look in
tree at
> >> test/Linker/type-unique-odr-a.ll.
> >>
> >> It's legal, e.g., to have the following in two separate files,
as long
> >> as the sequence of tokens is identical:
> >> --
> >> struct S { void foo(); };
> >> --
> >> However, the DISubprogram(isDefinition: false) for S::foo will not
be
> >> structurally equivalent (since the 'file:' node won't
match), so we
> >> currently end up with two metadata nodes.  The same thing happens
for
> >> the DICompositeType for `S` (and also applies to any fields).
> >>
> >> The checked-in testcase confirms that the description of `S` only
has
> >> one copy of `S::foo` in the DWARF output.  (The testcase
doesn't provide
> >> good enough coverage actually; it gets lucky because the
definition of
> >> S::foo is in the second file being linked (type-unique-odr-b.ll). 
If
> >> you reverse the order of the files, then `S` will have two copies
of
> >> `S::foo`.)
> >>
> >> 0005 solves the problem up front (and in both directions; I'll
add a RUN
> >> line to test/Linker/type-unique-odr-a.ll).
> >>
> >> I came across this because the testcase started failing with 0006.
The
> >> current (but not-quite-sufficient) logic relies on the resolve()
logic
> >> in DwarfDebug.cpp, which indirects through the merged
'retainedTypes:'
> >> map (the asymmetry in building the merged map explains why the
in-tree
> >> logic only works for one linking direction).
> >
> > Not sure I'm quite following.
> >
> > I think I understand the circumstance you're describing (indeed,
the
> type itself can be defined on different lines/files without violating the
> ODR, or its members, or, etc... ). But if the subprogram declaration is in
> a different file in one definition - that'd make the composite type
itself
> different (metadata uniquing - composite type references the subprogram,
> etc).
> >
> > But we should only every find one version of the type (all references
to
> the type should be via string-based dityperefs). Sure, we'll pick one
at
> random, but when/how would we get two?
>
> We get two because one of the modules contributes a definition of S::foo,
> and it may be pointing at the declaration from a version of S that
isn't
> blessed.  If you reverse the RUN line in the in-tree testcase you can see
> what happens, but here's a quick hand-written version:
> --
> !llvm.dbg.cus = !{!0, !4}
>
> !0 = distinct !DICompileUnit(file: !1, retainedTypes: !{!2})
> !1 = !DIFile(filename: "a.cpp")
> !2 = !DICompositeType(file: !1, name: "S", elements: !{!3},
>       identifier: "_ZTS1S")
> !3 = !DISubprogram(isDefinition: false, file: !1, name: "foo",
>       scope: !2, linkageName: "_ZN1S3fooEv")
>
> !4 = distinct !DICompileUnit(file: !5, retainedTypes: !{!6})
> !5 = !DIFile(filename: "b.cpp")
> !6 = !DICompositeType(file: !5, name: "S", elements: !{!7},
>       identifier: "_ZTS1S")
> !7 = !DISubprogram(isDefinition: false, file: !5, name: "foo",
>       scope: !6, linkageName: "_ZN1S3fooEv")
>
> define void @foo() !dbg !8 {
>   ret void
> }
>
> !8 = !DISubprogram(isDefinition: true, file: !5, name: "foo",
>       unit: !4, linkageName: "_Z3foov", declaration: !7)
> --
>
> If DwarfDebug picks !6 for the canonical "_ZTS1S", then
there's no
> problem.  !6 points at its function !7, and the definition !8 also
> points at !7.
>
> On the other hand, if DwarfDebug picks !2 for "_ZTS1S", then !2
will
> pull in !3 and !8 will still pull in !7.  The llvm-dwarfdebug output
> will emit both !3 and !7 inside of !2.
>
Ah, thanks for explaining it, I understand now. The problem is that the
definition reaches into the type blob via the declaration with a direct
reference.

We're going to want to abstract that reference away at some point to cross
the same boundary as for type references, I think - I imagine that's what
your patch/plan is to do?

As I think Reid chimed in here or elsewhere, he's going to need something
like that to cross the boundary into the opaque type blob for CodeView
output too, I imagine.

>
> >
> > >
> > >   - 0006: Remove DITypeRef (string-based references) and strip
down
> > >     'retainedTypes:'.
> > >
> > > Once this is done, I expect the bitcode block layout improvements
for
> > > lazy-loading of subprograms and composite types (steps 3 and 5
from
> > > this RFC) to be fairly straightforward.
> > >
> > >
> > >
> > >
> > > > On 2016-Mar-29, at 19:11, Eric Christopher <echristo at
gmail.com>
> wrote:
> > > >
> > > > I have no objections to any of this FWIW :)
> > > >
> > > > -eric
> > > >
> > > > On Tue, Mar 29, 2016 at 6:46 PM Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> > > >
> > > > > On 2016-Mar-22, at 20:11, Eric Christopher <echristo
at gmail.com>
> wrote:
> > > > >
> > > > >> On Tue, Mar 22, 2016 at 8:04 PM David Blaikie
<dblaikie at gmail.com>
> wrote:
> > > > >> +pcc, who had some other ideas/patch out for
improving memory
> usage of debug info
> > > > >> +Reid, who's responsible for the
windows/CodeView/PDB debug info
> which is motivating some of the ideas about changes to type emission
> > > > >
> > > > > So I discussed this with Adrian and Mehdi at the social
last
> Thursday and I'm getting set to finish the write up. I think it'll
have
> some bearing on this proposal as I think it'll change how we want to
take a
> look at the format of DISubprogram metadata a bit more.
> > > >
> > > > (The interesting bit here is to make a clearer split between
> > > > DISubprogram declarations (part of the type hierarchY) and
> > > > DISubprogram definitions (part of the code/line
table/variable
> > > > locations).  I think that'll end up being mostly
orthogonal to what
> > > > I'm trying to do.)
> > > >
> > > > > That said, most of it is orthogonal to the changes
Duncan is
> talking about here. Just puts the pressure on to get the other proposal
> written up.
> > > >
> > > > Which is now here:
> > > >
http://lists.llvm.org/pipermail/llvm-dev/2016-March/097773.html
> > > >
> > > > >> Baking into the IR more about types as units has
pretty direct
> overlap with Reid/CodeView/etc - so, yeah, that'll takes ome discussion
> (but, as you say, it's not in your immediate plan anyway, so we can
come
> back to that - but would be good for whoever gets there first to discuss it
> with the others)
> > > >
> > > > After thinking for a few days, I don't think this will
bake anything
> > > > new into the IR.  If anything it removes a few special
cases.
> > > >
> > > > More at the bottom.
> > > >
> > > > >>> Motivation
> > > > >>> =========> > > > >>>
> > > > >>> Based on some analysis Mehdi ran (ping him for
details), there
> are three
> > > > >>> (related) compile-time bottlenecks we're
seeing with `-flto=thin
> -g`:
> > > > >>>
> > > > >>>  a) Reading the large number of Metadata
bitcode records in the
> global
> > > > >>>     metadata block.  I'm talking about raw
`BitStreamer` calls
> here.
> > > > >>>
> > > > >>>  b) Creating unnecessary `DI*` instances (that
aren't relevant
> to code).
> > > > >>>
> > > > >>>  c) Emitting unnecessary `DI*` instances (that
aren't relevant
> to code).
> > > > >>>
> > > > >>> Here is my recollection of some peak memory
stats on a small
> testcase
> > > > >>> during thin-LTO, which should be a decent
indicator of (b):
> > > > >>>
> > > > >>>   - ~150MB: DILocation
> > > > >>>   - ~100MB: DISubprogram
> > > > >>>   - ~70MB: DILocalVariable
> > > > >>>   - ~50MB: (cumulative) DIType descendents
> > > > >>>
> > > > >>> It looks, suprisingly, like types are not the
primary bottleneck.
> > > >
> > > > (Probably wrong for (a), BTW.  Caveats matter.)
> > > >
> > > > >>> There are caveats:
> > > > >>>
> > > > >>>   - `DISubprogram` declarations -- member
function descriptors
> -- are
> > > > >>>     part of the type hierarchy.
> > > > >>>   - Most of the type hierarchy gets uniqued at
parse time.
> > > > >>>   - As a result, these data are a poor
indicator for (a).
> > > >
> > > > ((a) is the main bottleneck for compile-time of -flto=thin
(since
> it's
> > > > quadratic in the number of files).  (b) only affects memory.
Also
> > > > important, but at least it scales linearly.)
> > > >
> > > > >>> Even so, non-types are substantial.
> > > > >>>
> > > > >>> Proposal
> > > > >>> =======> > > > >>>
> > > > >>> Short version
> > > > >>> -------------
> > > > >>>
> > > > >>>  4. Remove `DICompositeType`s from
`retainedTypes:`, similar to
> (2).
> > > >
> > > > This is the part that's relevant to the new RFC Eric
just posted.
> > > >
> > > > >>> Long version
> > > > >>> -------------
> > > > >>>
> > > > >>>  4. Implement my proposal to remove the
`DICompositeType` name
> map from
> > > > >>>     `retainedTypes:`.
> > > > >>>
> > > > >>>
>
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160125/327936.html
> > > > >>>
> > > > >>>     Similar to (2) above, this will naturally
filter the types
> that get
> > > > >>>     linked in to the ones actually used by the
code being linked.
> > > > >>>
> > > > >>>     It should also allow the reader to skip
records for types
> that have
> > > > >>>     already been loaded in the main module.
> > > >
> > > > The essential things I want to accomplish with this part:
> > > >
> > > >   - Make `type:` operands less special: instead of
referencing types
> > > >     indirectly through MDString, point directly at the type
node.
> > > >
> > > >   - Stop using `retainedTypes:` by default (only for -gfull,
etc.).
> > > >
> > > >   - Avoid blowing up memory in -flto=full (which converting
MDString
> > > >     references back to pointers would do naively, through
> > > >     re-introducing cycles).  Note that this needs to be
handled
> > > >     somehow at BitcodeReader time.
> > > >
> > > > After chatting with Eric, I don't think this conflicts
at all with
> the
> > > > other RFC.  Unifying the `type:` operands might actually
help both.
> > > >
> > > > One good point David mentioned last week was that we
don't want to
> > > > teach the IR any more about types.  Rather than inventing
some new
> > > > context (as I suggested originally), I figure LTO plugins
can just
> > > > pass a (StringRef -> DIType*) map to the BitcodeReader,
and the
> Module
> > > > itself doesn't need to know anything about it.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160415/1a9d085f/attachment.html>