<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 15, 2016 at 2:27 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
> On 2016-Apr-15, at 10:27, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Fri, Apr 15, 2016 at 9:50 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> Since I haven't heard any objections to the direction, I'm planning to<br>
> commit this (step 4) when I find some cycles; likely over the weekend.<br>
><br>
> To make this more concrete (in case someone is silently concerned) I've<br>
> posted my WIP patches below. They apply cleanly to r266414. There are<br>
> a few mechanical changes missing that are tracked in the commit<br>
> messages (such as LangRef, or new tests in some cases). To get<br>
> `ninja check` to pass after the final commit you'll need to run the<br>
> attached upgrade script. The clang changes are truly uninteresting so<br>
> I've left them out.<br>
><br>
> - 0001/2: Prep commits I need to flush out (sorry for the noise).<br>
> - 0003: Add an explicit type map for ODR type uniquing.<br>
> - 0004: Prep.<br>
> - 0005: Add ODR type uniquing of members.<br>
><br>
> What's this ^ for?<br>
><br>
> 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)<br>
><br>
> 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?<br>
<br>
</span>This is a strange one; I probably should have called it out actually.<br>
It's a bugfix vs. ToT; and without it, 0006 regresses DWARF output.<br>
<br>
File and line can change without violating ODR, which means that we<br>
end up with duplicate members. I think I may have written something<br>
up in the WIP commit message for 0005, but also have a look in tree at<br>
test/Linker/type-unique-odr-a.ll.<br>
<br>
It's legal, e.g., to have the following in two separate files, as long<br>
as the sequence of tokens is identical:<br>
--<br>
struct S { void foo(); };<br>
--<br>
However, the DISubprogram(isDefinition: false) for S::foo will not be<br>
structurally equivalent (since the 'file:' node won't match), so we<br>
currently end up with two metadata nodes. The same thing happens for<br>
the DICompositeType for `S` (and also applies to any fields).<br>
<br>
The checked-in testcase confirms that the description of `S` only has<br>
one copy of `S::foo` in the DWARF output. (The testcase doesn't provide<br>
good enough coverage actually; it gets lucky because the definition of<br>
S::foo is in the second file being linked (type-unique-odr-b.ll). If<br>
you reverse the order of the files, then `S` will have two copies of<br>
`S::foo`.)<br>
<br>
0005 solves the problem up front (and in both directions; I'll add a RUN<br>
line to test/Linker/type-unique-odr-a.ll).<br>
<br>
I came across this because the testcase started failing with 0006. The<br>
current (but not-quite-sufficient) logic relies on the resolve() logic<br>
in DwarfDebug.cpp, which indirects through the merged 'retainedTypes:'<br>
map (the asymmetry in building the merged map explains why the in-tree<br>
logic only works for one linking direction).<br></blockquote><div><br></div><div>Not sure I'm quite following.<br><br>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). <br><br>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?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
><br>
> - 0006: Remove DITypeRef (string-based references) and strip down<br>
> 'retainedTypes:'.<br>
><br>
> Once this is done, I expect the bitcode block layout improvements for<br>
> lazy-loading of subprograms and composite types (steps 3 and 5 from<br>
> this RFC) to be fairly straightforward.<br>
><br>
><br>
><br>
><br>
> > On 2016-Mar-29, at 19:11, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br>
> ><br>
> > I have no objections to any of this FWIW :)<br>
> ><br>
> > -eric<br>
> ><br>
> > On Tue, Mar 29, 2016 at 6:46 PM Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> ><br>
> > > On 2016-Mar-22, at 20:11, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br>
> > ><br>
> > >> On Tue, Mar 22, 2016 at 8:04 PM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> > >> +pcc, who had some other ideas/patch out for improving memory usage of debug info<br>
> > >> +Reid, who's responsible for the windows/CodeView/PDB debug info which is motivating some of the ideas about changes to type emission<br>
> > ><br>
> > > 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.<br>
> ><br>
> > (The interesting bit here is to make a clearer split between<br>
> > DISubprogram declarations (part of the type hierarchY) and<br>
> > DISubprogram definitions (part of the code/line table/variable<br>
> > locations). I think that'll end up being mostly orthogonal to what<br>
> > I'm trying to do.)<br>
> ><br>
> > > 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.<br>
> ><br>
> > Which is now here:<br>
> > <a href="http://lists.llvm.org/pipermail/llvm-dev/2016-March/097773.html" rel="noreferrer" target="_blank">http://lists.llvm.org/pipermail/llvm-dev/2016-March/097773.html</a><br>
> ><br>
> > >> 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)<br>
> ><br>
> > After thinking for a few days, I don't think this will bake anything<br>
> > new into the IR. If anything it removes a few special cases.<br>
> ><br>
> > More at the bottom.<br>
> ><br>
> > >>> Motivation<br>
> > >>> ==========<br>
> > >>><br>
> > >>> Based on some analysis Mehdi ran (ping him for details), there are three<br>
> > >>> (related) compile-time bottlenecks we're seeing with `-flto=thin -g`:<br>
> > >>><br>
> > >>> a) Reading the large number of Metadata bitcode records in the global<br>
> > >>> metadata block. I'm talking about raw `BitStreamer` calls here.<br>
> > >>><br>
> > >>> b) Creating unnecessary `DI*` instances (that aren't relevant to code).<br>
> > >>><br>
> > >>> c) Emitting unnecessary `DI*` instances (that aren't relevant to code).<br>
> > >>><br>
> > >>> Here is my recollection of some peak memory stats on a small testcase<br>
> > >>> during thin-LTO, which should be a decent indicator of (b):<br>
> > >>><br>
> > >>> - ~150MB: DILocation<br>
> > >>> - ~100MB: DISubprogram<br>
> > >>> - ~70MB: DILocalVariable<br>
> > >>> - ~50MB: (cumulative) DIType descendents<br>
> > >>><br>
> > >>> It looks, suprisingly, like types are not the primary bottleneck.<br>
> ><br>
> > (Probably wrong for (a), BTW. Caveats matter.)<br>
> ><br>
> > >>> There are caveats:<br>
> > >>><br>
> > >>> - `DISubprogram` declarations -- member function descriptors -- are<br>
> > >>> part of the type hierarchy.<br>
> > >>> - Most of the type hierarchy gets uniqued at parse time.<br>
> > >>> - As a result, these data are a poor indicator for (a).<br>
> ><br>
> > ((a) is the main bottleneck for compile-time of -flto=thin (since it's<br>
> > quadratic in the number of files). (b) only affects memory. Also<br>
> > important, but at least it scales linearly.)<br>
> ><br>
> > >>> Even so, non-types are substantial.<br>
> > >>><br>
> > >>> Proposal<br>
> > >>> ========<br>
> > >>><br>
> > >>> Short version<br>
> > >>> -------------<br>
> > >>><br>
> > >>> 4. Remove `DICompositeType`s from `retainedTypes:`, similar to (2).<br>
> ><br>
> > This is the part that's relevant to the new RFC Eric just posted.<br>
> ><br>
> > >>> Long version<br>
> > >>> -------------<br>
> > >>><br>
> > >>> 4. Implement my proposal to remove the `DICompositeType` name map from<br>
> > >>> `retainedTypes:`.<br>
> > >>><br>
> > >>> <a href="http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160125/327936.html" rel="noreferrer" target="_blank">http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160125/327936.html</a><br>
> > >>><br>
> > >>> Similar to (2) above, this will naturally filter the types that get<br>
> > >>> linked in to the ones actually used by the code being linked.<br>
> > >>><br>
> > >>> It should also allow the reader to skip records for types that have<br>
> > >>> already been loaded in the main module.<br>
> ><br>
> > The essential things I want to accomplish with this part:<br>
> ><br>
> > - Make `type:` operands less special: instead of referencing types<br>
> > indirectly through MDString, point directly at the type node.<br>
> ><br>
> > - Stop using `retainedTypes:` by default (only for -gfull, etc.).<br>
> ><br>
> > - Avoid blowing up memory in -flto=full (which converting MDString<br>
> > references back to pointers would do naively, through<br>
> > re-introducing cycles). Note that this needs to be handled<br>
> > somehow at BitcodeReader time.<br>
> ><br>
> > After chatting with Eric, I don't think this conflicts at all with the<br>
> > other RFC. Unifying the `type:` operands might actually help both.<br>
> ><br>
> > One good point David mentioned last week was that we don't want to<br>
> > teach the IR any more about types. Rather than inventing some new<br>
> > context (as I suggested originally), I figure LTO plugins can just<br>
> > pass a (StringRef -> DIType*) map to the BitcodeReader, and the Module<br>
> > itself doesn't need to know anything about it.<br>
<br>
</div></div></blockquote></div><br></div></div>