<div dir="ltr">Hi James,<div><br></div><div>Sorry for the delay, the holidays, you know. ;)</div><div><br></div><div>So, I haven't looked thoroughly at your patch, but it looks a lot more like lazy evaluation. Though I may be completely wrong about all of it, there were things that popped to my eyes:</div>
<div><br></div><div>First, you're passing the value materializer all over the place. It's normally ok to do that in the case where the object being passed has a context that no one else has and that it's dependent on the context and the path it traced before getting to the call, but that's not the case. The value materializer is very simple, nothing more than a helper, that gets created in the constructor of another class. It seems you're doing that to allow extension for different kinds of materializers, though there aren't any yet (and may never have). Shouldn't we let the generalization for when we have a concrete case in hand.</div>
<div><br></div><div>Second, you're using "friend class ValueMaterializerTy" on ModuleLinker. A friend of mine used to say that when you use "friend", you're either incredibly clever or doing something wrong. So far, every time I used it, I was doing something wrong. If that class is meant to be extended (see above), this will complicate things. Even if not, they're normally an indication that you're class hierarchy is ill defined.</div>
<div><br></div><div>Lastly, you seem to have fixed a bug since last patch, but I see no signs of a test, or even a hint on what kind of bug it was that got fixed. I understand that it may be a little hard to test lazy linking with an IR test, but you can always write a unittest, or enable debug mode and monitor output (like the loop vectorizer does).</div>
<div><br></div><div style>My tuppence, take it with a bit of salt.</div><div style><br></div><div style>cheers,</div><div style>--renato</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">
On 23 May 2013 16:10, James Molloy <span dir="ltr"><<a href="mailto:James.Molloy@arm.com" target="_blank">James.Molloy@arm.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-GB" link="blue" vlink="purple">
<div>
<p class="MsoNormal">Hi,<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">A while back I submitted a patch to improve linker performance when lazily linking functions (r178130). It got reverted in r178156 by Bill Wendling because it broke a smooshlab buildbot. I didn’t notice for a while because I wasn’t on the
fail mail and I fail at reading llvm-commits regularly enough <span style="font-family:Wingdings">
L</span><u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">I replicated the failure smooshlab found, and realised there was a flaw in how I’d implemented lazy linking. I’ve attached a new patch with a changed approach, and would appreciate a review. The new approach is to extend RemapInstruction
and friends to take an optional extra class ValueMaterializer, which can be subclassed to generate Value*s on demand. This allows the linker to only generate Function*s when it needs to, and increases performance by around 45% when linking large modules with
many lazy linked functions of which few get used (such as CL kernels and built in function libraries).<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">This patch passes the LTO/Gold build it failed on before, and passes the LNT nightly tests.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Please review!<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Cheers,<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">James <u></u><u></u></p>
</div>
<br>
<font face="Arial" color="Black">-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents
to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.<br>
</font>
</div>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>