<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">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:0 0 0 .8ex;border-left:1px #ccc 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><div>Sure thing. No rush - my review approach is fairly "fire and forget until it comes around again".</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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">
<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><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*).<br>For each imported subprogram, I'd imagine we would clone the subprogram, and any types (as declarations) that we need.<br> If an imported type requires another type, I imagine the act of importing the first type would import the second<br> If any imported type is referenced by a type ref/string name, I expect to resolve that and import it<br><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><br>* What's the longer case there that says something about "for non post-pass metadata linking" (1445-1466)?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc 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">
<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><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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc 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">
<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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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">
<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 class="h5"><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>
<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>
<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>
<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>
<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 class=""><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>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">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></blockquote></div><br></div></div>