[PATCH] Improve linker performance (v2)

[James Molloy]

Hi Renato,

Thanks for the review! You’re right about the lazy evaluation. The last patch was also doing lazy evaluation too, but in a less intrusive way.

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

I see where you're coming from, but do disagree. The RemapInstruction function and friends are already generic and used in many places - one of which is the linker. The ValueMaterializer class is a way of parameterizing the RemapInstruction suite of functions. As such, it does need to be generic and can't be linker-specific.

I understand that there's only one use of this functionality so far, but considered that (a) demand-value-generation may be useful to other (potentially out of tree) consumers, because the RemapInstruction interface is used widely and (b) RemapInstruction is sufficiently complex that rewriting it just for the linker would be worse practice.

I'm open to suggestions / further argument about that though :)

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

I agree. Actually ValueMaterializerTy is what in C++11 would be defined as a final class. It extends ValueMaterializer and is the last implementation in the hierarchy. It is a concrete class specific to the linker, and requires access to multiple pieces of the linker state, which is why I saw fit to grant it friend status.

I can change this to pass references to everything it needs in the constructor however, if you feel that's a better solution!

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

This is true, apologies. I'll attempt to distill down a regression test (it took quite a bit of distilling to just get it to a state where I could understand what was going on :()

Cheers,

James

From: Renato Golin [mailto:renato.golin at linaro.org]
Sent: 28 May 2013 10:07
To: James Molloy
Cc: llvm-commits; Renato Golin
Subject: Re: [PATCH] Improve linker performance (v2)

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 ☹

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


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