<div dir="ltr">Hi Duncan,<div><br></div><div>I just scanned it briefly right now - do you have an updated version of the patch addressing Mehdi's comments? One minor comment so far:</div><div><br></div><div><div>+void ValueEnumerator::purgeFunctionMetadata() {</div><div>+ while (MDs.size() > NumBaseMDs) {</div><div>+ //MetadataMap.erase(MDs.back());</div><div><br></div><div>Commented line?</div><div><br></div></div><div>Thanks,</div><div>Teresa</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 29, 2016 at 10:23 PM, Mehdi Amini 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 class="HOEnZb"><div class="h5"><br>
> On Mar 28, 2016, at 3:42 PM, Mehdi Amini <<a href="mailto:mehdi.amini@apple.com">mehdi.amini@apple.com</a>> wrote:<br>
><br>
>><br>
>> On Mar 28, 2016, at 7:44 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>><br>
>><br>
>>> On 2016-Mar-27, at 17:41, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>>><br>
>>> Whenever metadata is only referenced by a single function, emit the<br>
>>> metadata just in that function block. This should improve lazy-loading<br>
>>> by reducing the amount of metadata in the global block.<br>
>>><br>
>>> For now, this should catch all DILocations, and anything else that<br>
>>> happens to be referenced only by a single function.<br>
>>><br>
>>> It's also a first step toward a couple of possible future directions<br>
>>> (which this commit does *not* implement):<br>
>>><br>
>>> 1. Some debug info metadata is only referenced from compile units and<br>
>>> individual functions. If we can drop the link from the compile<br>
>>> unit, this optimization will get more powerful.<br>
>>><br>
>>> 2. Any uniqued metadata that isn't referenced globally can in theory be<br>
>>> emitted in every function block that references it (trading off<br>
>>> bitcode size and full-parse time vs. lazy-load time).<br>
>>><br>
>>> Note: the only change on the reader side is cautious error checking.<br>
>>> The metadata stored in function blocks gets purged after parsing each<br>
>>> function, which means unresolved forward references will get lost.<br>
>>> Since all the global metadata should have already been resolved by the<br>
>>> time we get to the function metadata blocks we just need to check for<br>
>>> that case. (If for some reason we need this, the fix is to store<br>
>>> about-to-be-dropped unresolved nodes in MetadataList::shrinkTo until<br>
>>> they can be handled succesfully by a future call to<br>
>>> MetadataList::tryToResolveCycles.)<br>
>>><br>
>>> <0001-Bitcode-Try-to-emit-metadata-in-function-blocks-v3.patch><br>
>><br>
>> I realized that it's straightforward to use llvm-bcanalyzer to test that<br>
>> this works correctly. Attaching a new version of the patch with a<br>
>> testcase.<br>
><br>
><br>
> I wanted to test it locally, but can't ThinLTO-link llvm-tblgen, it crashes with "error: Invalid record: metadata strings corrupt offset"<br>
><br>
> Some other comments (while I'm still trying to make sense of organizeMetadata()):<br>
><br>
> + struct MDIndex {<br>
><br>
> one-line doxygen could be welcome<br>
><br>
> + struct MDRange {<br>
><br>
> same<br>
><br>
> - unsigned NumModuleMDs;<br>
> + unsigned NumModuleMDStrings = 0;<br>
> + unsigned NumBaseMDs = 0;<br>
> + unsigned NumMDStrings = 0;<br>
><br>
> Same<br>
><br>
><br>
> /// Purge function metadata.<br>
> void purgeFunctionMetadata();<br>
><br>
> Doxygen that are 1-1 matching to the function name are useless, either there is something more useful to say, or it could be dropped if the name makes what it is intended for obvious.<br>
><br>
><br>
><br>
> + // Check whether this is a new function.<br>
> + if (Insertion.first->second.updateFunction(F))<br>
> + if (auto *N = dyn_cast<MDNode>(MD))<br>
> + NeedToDropFunctions.push_back(N);<br>
><br>
> The dyn_cast is not clear to me?<br>
><br>
><br>
> + return std::make_tuple(LHS.F, !isa<MDString>(MDs[<a href="http://LHS.ID" rel="noreferrer" target="_blank">LHS.ID</a> - 1]), <a href="http://LHS.ID" rel="noreferrer" target="_blank">LHS.ID</a>) <<br>
> + std::make_tuple(RHS.F, !isa<MDString>(MDs[<a href="http://RHS.ID" rel="noreferrer" target="_blank">RHS.ID</a> - 1]), <a href="http://RHS.ID" rel="noreferrer" target="_blank">RHS.ID</a>);<br>
><br>
> Things like the "- 1" offset that is leaking here seems like a missing abstraction. Not that it seems to be widespread, but could be hidden in a member of MDIndex.<br>
><br>
><br>
> + // Return early if nothing happened.<br>
> + if (!Order.back().F && !isa<MDString>(MDs[Order.front().ID - 1]))<br>
> + return;<br>
><br>
> I'm glad there is a comment, without it I wouldn't know what's going on here!<br>
<br>
<br>
<br>
</div></div>I don't have much to add. The reader/writer part is trivial, there is only the ValueEnumerator which might now be intuitive on the first sight. Just a good high level description of what is achieved with the two struct (MDindex and MDRang) should be enough.<br>
<br>
So LGTM.<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<br>
--<br>
Mehdi<br>
<br>
<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>
</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>