<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 21, 2016 at 5:46 PM, Teresa Johnson <span dir="ltr"><<a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Tue, Mar 15, 2016 at 2:12 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Tue, Mar 15, 2016 at 1:48 PM, Teresa Johnson via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 10, 2016 at 3:16 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">dblaikie added inline comments.<br></blockquote><div><br></div><div>Thanks for the comments! I had hoped to address these all before I left town, but ran out of time. A few responses below, will address the rest when I get back!</div></div></div></div></blockquote><div><br></div></span><div>Sure thing. No rush - my review approach is fairly "fire and forget until it comes around again".</div><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Linker/IRMover.cpp:1386<br>
@@ +1385,3 @@<br>
+ // that is needed, and we should only invoke this for needed composite types.<br>
+ assert(RMI != RetainMap.end());<br>
+ // If we already created a new composite type declaration, use it.<br>
----------------<br>
I'm not sure this is true. Here's a type you might have to import that is not in the retained types list:<br>
<br>
namespace {<br>
struct foo { };<br>
void f(foo) { }<br>
}<br>
struct bar { };<br>
void f(bar) { }<br>
void f() {<br>
f(foo());<br>
f(bar());<br>
}<br>
<br>
The 'foo' type is not in the retained types list, but it may need to be imported if 'f(foo)' is imported, yes?<br></blockquote><div><br></div></span><div>While foo isn't in the retained types list, it is imported as it is reached via the DISubprogram for f(foo) and we do pull it in (confirmed with the above example and this patch). Perhaps the comment could be clarified to refer to needed retained types, not composite types, since this is only dealing with types reached via the retained types list.</div></div></div></div></blockquote><div><br></div></span><div>Hmm, following the algorithm is a bit difficult for me. The code appears more complex than I would've expected. <br><br>Perhaps I can explain the algorithm I would expect & you can point out where I've oversimplified/got confused.<br><br><br></div><div>Basically what I'd expect is a loop over imported functions (OK, I see something like that - a loop over subprograms, skipping any "unneeded" subprograms around line 1474*).</div></div></div></div></blockquote><div> </div></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>* What's the longer case there that says something about "for non post-pass metadata linking" (1445-1466)?</div></div></div></div></blockquote><div><br></div><div>Moving this question up here since the answer will hopefully help explain what is going on. For ThinLTO function importing there are two ways of handling metadata that are supported, described below. There are some tradeoffs based on how the importing behaves, and currently the former is used by llvm-link and the latter used by the function importing pass, but I need to do more experiments with large source files to see what is the best approach for some of our large apps, for example.</div><div><br></div><div>The post-pass case was implemented first since I anticipated function importing to be iterative (import a single function at a time from a module, perhaps interleaved with functions imported from other modules). In that case the metadata is not materialized when we materialize and link in each imported source function. After we are done with function importing, we go back and materialize a single time the (module-level) metadata for each source module we imported from, and at that point link in all needed metadata (which requires some bookkeeping and suturing of the metadata referenced by the already-imported functions).</div><div><br></div><div>When we are doing a single batch importing of functions from each module, we instead materialize the module-level metadata before we link in the imported functions. This is how full LTO behaves as well. In this case, as with full LTO, when the function bodies are linked in any metadata reached from those functions (e.g. the DISubprogram and descendants) is naturally linked in (see the MapMetadata call in IRLinker::linkFunctionBody). However, we still need to link in any types reached from the retained types list and referenced via name by any of the imported subprograms. That happens when we link in the CU named metadata.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>For each imported subprogram, I'd imagine we would clone the subprogram, and any types (as declarations) that we need.<br></div></div></div></div></blockquote><div><br></div></span><div>As noted above, for the non-post-pass case (as with full LTO), this naturally done for direct referenced types when the function bodies are linked in. For post-pass, we need to do this here via the findReachedCompositeTypes call for each needed DISubprogram.</div></div></div></div></blockquote><div><br></div><div>As I was going back to comment this more clearly I realized that for post-pass metadata linking we have also already mapped in any needed DISubprogram and any other types reached via direct reference. This is because we invoke MapMetadata on the CU's subprogram list just above. Confirmed that in the post-pass case we do have all such reached metadata already in the ValueMap just as we do for the non-postpass case. I was able to simplify the code to always use what was previously guarded by if (!IsMetadataLinkingPostpass), and removed findReachedCompositeTypes().</div><div><br></div><div>I have addressed the other comments via improved names and/or comments, and removed the offset and base type from the type declarations being created (added a note about size and alignment).</div><div><br></div><div>Will upload the new patch momentarily.</div><div><br></div><div>Thanks,</div><div>Teresa</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> If an imported type requires another type, I imagine the act of importing the first type would import the second<br></div></div></div></div></blockquote><div><br></div></span><div>Yes when direct referenced.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> If any imported type is referenced by a type ref/string name, I expect to resolve that and import it<br></div></div></div></div></blockquote><div><br></div></span><div>That is what we do via the RetainWorkList.</div><span class=""><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>But it seems like that's not quite the way the algorithm works (notably - importing string and direct reference types aren't handled uniformly by the type importing step - they're deferred into a list then handled separately, but not quite understanding why?)<br></div></div></div></div></blockquote><div><br></div></span><div>So that this is handled consistently regardless of whether we are doing post-pass or non-post-pass metadata linking.</div><div> </div><div>Hopefully that clarifies how this works, but let me know if not!</div><span class=""><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br><br></div><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Linker/IRMover.cpp:1398<br>
@@ +1397,3 @@<br>
+ Composite->getFile(), Composite->getLine(), NewScope, NewBaseType,<br>
+ Composite->getSizeInBits(), Composite->getAlignInBits(),<br>
+ Composite->getOffsetInBits(), Composite->getFlags() | DINode::FlagFwdDecl,<br>
----------------<br>
Pretty sure you don't need the base type, size, alignment, and offset in a declaration - check equivalent declarations that are generated by clang? (check that the declarations this code creates look like natural declarations in the original source language, etc)<br></blockquote><div><br></div></span><div>They do seem to have size and alignment, but not baseType or offset. Will fix. Here's an example:</div><div><br></div><div> !1215 = !DICompositeType(tag: DW_TAG_class_type, name: "DOMText", scope: !524, file: !1216, line: 90, size: 64, align: 64, flags: DIFlagFwdDecl, identifier: "_ZTSN11xercesc_2_57DOMTextE")</div></div></div></div></blockquote><div><br></div></span><div>Oh, bother. Hrm. I wonder how those got there. I don't think they should be. If we only have a declaration for a type there's no way we know the correct size or alignment in general.</div></div></div></div></blockquote><div><br></div></span><div>I can suppress these when I create the decl, but that seems inconsistent with what is currently emitted. WDYT? </div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Linker/IRMover.cpp:1411<br>
@@ +1410,3 @@<br>
+Metadata *<br>
+IRLinker::importType(Metadata *Ty,<br>
+ DenseMap<DICompositeType *, DICompositeType *> &RetainMap,<br>
----------------<br>
This name seems a bit misleading. If I'm reading the code correctly, this is used for any type's scope context - which might not be a type at all. (it could be a namespace, for example) - and I assume the non-type cases come out in the "map the type metadata normally" part?<br></blockquote><div><br></div></span><div>Correct, it will also handle DINamespace which will be mapped normally via MapMetadata. I will rename this to something better. </div></div></div></div></blockquote></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Linker/IRMover.cpp:1427<br>
@@ +1426,3 @@<br>
+<br>
+void IRLinker::linkImportedCompileUnit() {<br>
+ NamedMDNode *SrcCompileUnits = SrcM.getNamedMetadata("<a href="http://llvm.dbg.cu" rel="noreferrer" target="_blank">llvm.dbg.cu</a>");<br>
----------------<br>
Name seems a bit off - this links in potentially multiple compile units, not just a single one.<br></blockquote><div><br></div></span><div>Right, will make the name plural.</div><div><br></div><div>This is as far as I got. Will address the rest when I am back in town Monday!</div><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Linker/IRMover.cpp:1444<br>
@@ +1443,3 @@<br>
<span>+ std::vector<DICompositeType *> RetainWorklist;<br>
+ if (!IsMetadataLinkingPostpass) {<br>
</span>+ for (DIType *Ty : CU->getRetainedTypes()) {<br>
----------------<br>
What's postpass metadata linking? (what's the alternative?)<br></blockquote></div></div></div></div></div></div></div></blockquote></div></div></div></blockquote><div><br></div></div></div><div>Hopefully answered this above.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Linker/IRMover.cpp:1445<br>
@@ +1444,3 @@<br>
+ if (!IsMetadataLinkingPostpass) {<br>
+ for (DIType *Ty : CU->getRetainedTypes()) {<br>
+ // For non-postpass metadata linking, any metadata referenced<br>
----------------<br>
I'm not really following why we need to iterate the retained types list - wouldn't we import everything starting from the subprograms we needed to import, not the retained types list?<br></blockquote></div></div></div></div></div></div></div></blockquote></div></div></div></blockquote><div><br></div></span><div>Hopefully answered above too.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Linker/IRMover.cpp:1534<br>
@@ +1533,3 @@<br>
+ stripNullSubprograms(NewCU);<br>
+ DestCompileUnits->addOperand(NewCU);<br>
+ }<br>
----------------<br>
What if nothing was imported from this CU?<br></blockquote></div></div></div></div></div></div></div></blockquote></div></div></div></blockquote><div><br></div></span><div>We only invoke the IRMover during importing if we are importing something from that module. </div><span class=""><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
It seems to me we'd want to start at the imported functions, follow their DISubprograms, and import that way, rather than walking all the CUs and subprograms - maybe?<br></blockquote></div></div></div></div></div></div></div></blockquote></div></div></div></blockquote><div><br></div></span><div>Hopefully answered above too.</div><span class=""><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
<br>
<a href="http://reviews.llvm.org/D16440" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16440</a><br>
<br>
<br>
<br>
</blockquote></div></div></div><br><br clear="all"><span><div><br></div>-- <br><div><span style="font-family:Times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px">Teresa Johnson |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px"> Software Engineer |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px"> <a href="tel:408-460-2413" value="+14084602413" target="_blank">408-460-2413</a></td></tr></tbody></table></span></div>
</span></div></div>
<br></div></div><span>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></span></blockquote></div><br></div></div>
</blockquote></span></div><span class=""><br><br clear="all"><div><br></div>-- <br><div><span style="font-family:Times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px">Teresa Johnson |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px"> Software Engineer |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px"> <a href="tel:408-460-2413" value="+14084602413" target="_blank">408-460-2413</a></td></tr></tbody></table></span></div>
</span></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><span style="font-family:Times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px">Teresa Johnson |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px"> Software Engineer |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px"> 408-460-2413</td></tr></tbody></table></span></div>
</div></div>