[PATCH] Improve linker performance (v2)

[Renato Golin]

Hi James,

Sorry for the delay, the holidays, you know. ;)

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:

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.

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.

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).

My tuppence, take it with a bit of salt.

cheers,
--renato




On 23 May 2013 16:10, James Molloy <James.Molloy at arm.com> wrote:

>  Hi,****
>
> ** **
>
> 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 L****
>
> ** **
>
> 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).****
>
> ** **
>
> This patch passes the LTO/Gold build it failed on before, and passes the
> LNT nightly tests.****
>
> ** **
>
> Please review!****
>
> ** **
>
> Cheers,****
>
> ** **
>
> James ****
>
> -- 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.
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130528/51dd6231/attachment.html>