[PATCH] D19351: ELF: Add initial ThinLTO support.

[Peter Collingbourne via llvm-commits]

On Thu, May 12, 2016 at 9:36 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:

>
> On May 12, 2016, at 9:25 PM, Peter Collingbourne <peter at pcc.me.uk> wrote:
>
> On Thu, May 12, 2016 at 8:29 PM, Mehdi Amini <mehdi.amini at apple.com>
> wrote:
>
>>
>> On May 12, 2016, at 6:29 PM, Peter Collingbourne <peter at pcc.me.uk> wrote:
>>
>> Thanks Rafael.
>>
>> On Thu, May 12, 2016 at 4:32 PM, Rafael Espíndola <
>> rafael.espindola at gmail.com> wrote:
>>
>>> On 12 May 2016 at 16:13, Peter Collingbourne <peter at pcc.me.uk> wrote:
>>> > pcc added a comment.
>>> >
>>> > I have updated the patch. I just want to make sure that people are
>>> happy with the situation around the ThinLTO API. I've been working on an
>>> LTO/ThinLTO API that could be used by both the gold plugin and lld which
>>> should address Mehdi's concerns, but it will take some time until that's
>>> ready.
>>> >
>>>
>>> Sorry I dropped out of this thread.
>>>
>>> I don't known a lot about ThinLTO, so most comments will be generic
>>> about LTO.
>>>
>>> The way the existing lib/LTO is implemented is fundamentally different
>>> from what we do in the gold plugin or lld. We want to make sure we
>>> don't even read and copy symbols if the linker decided to not keep it.
>>> The lib/LTO interface is designed to talk about symbols in the merged
>>> module.
>>>
>>
>> Right, I certainly don't want to use any of the existing lib/LTO anywhere
>> we're not using it now. The new API I'm working on deals with symbols in
>> individual modules.
>>
>> Having said that, I dislike duplicated code as much as anyone else.
>>> That is why I really don't want lld calling into Linker::linkModules:
>>> it duplicates symbol resolution. The same goes for the call to
>>> addPassesToEmitFile. We really should figure out a single place to
>>> populate the pipeline.
>>>
>>> If it is possible for lld/gold/LTO to share more code, excellent but:
>>>
>>> * I would probably not put that in lib/LTO. Lets keep that just for
>>> the existing api.
>>>
>>
>> Putting the new API in some other lib makes sense to me.
>>
>>
>>
>> Can you elaborate on the rational for that? It does not seem obvious to
>> me (i.e. that's where I'd go for any new API that is dedicated to LTO).
>>
>
> I think the idea is that we'd have one API with a resolution-based
> interface, and a separate API with a resolution-free interface. We might
> want to call this new API "LTO" and rename the existing one to something
> else, to make it clear which one people should try to use. Pick your
> favourite bikeshed colour.
>
>>
>>
>>
>>
>>
>>> * It still seems a bit early for this. We still having more basic
>>> things to finish in lld and the thinLTO api is still moving.
>>>
>>
>> Regarding the new API, I think there’s a secondary reason why we might
>> want to introduce it sooner rather than later. That reason is that we plan
>> to store symbol resolutions in a combined module index, which basically
>> describes which module contains the prevailing definition of each function.
>> Getting this right is basically a critical correctness issue, as we could
>> otherwise for example import the definition of a weak symbol instead of the
>> prevailing strong symbol. I also understand that Teresa plans to use this
>> information in distributed backends.
>>
>> One way we could implement this could be to expose gold's symbol
>> resolutions to the ThinLTO importer in some form. The problem with this is
>> that it adds more complexity in a gold-specific layer, when we know that we
>> will eventually need to deal with that in every linker. Or we could
>> implement symbol resolution ourselves in the importer via the linkage info
>> in the summaries, but as I’m sure you are aware, this would be duplicating
>> logic from the linker. That’s why I feel it’s best to have the importer
>> deal with linker-agnostic symbol resolution info.
>>
>> Getting this right in the gold plugin is basically orthogonal to whether
>> we implement anything in lld. So, the course of action I was considering was
>> 1) Get the new interface in place, and port the gold plugin to it;
>> 2) (in parallel with 1) Polish regular LTO in lld as necessary;
>> 3) Port lld to the new interface.
>> 4) (low priority for me, but maybe Mehdi can help) Port libLTO to the new
>> interface.
>>
>>
>> It absolutely makes no sense to me to develop a new API in LLVM that
>> refactor gold and lld without taking into account the current
>> ThinLTOCodeGenerator.
>>
>
> Oh, I certainly agree, which is why the API will permit resolution-free
> APIs such as *LTOCodeGenerator to be implemented in terms of it. As I
> mentioned on IRC, this could work by teaching *LTOCodeGenerator to compute
> symbol resolutions by itself and passing those resolutions to the new
> interface.
>
> This is an issue of priorities for me. As I mentioned, I haven't made any
> decisions yet. I understand that libLTO is important for you, but I have no
> direct interest in it, which by default makes porting that API a low
> priority for me, but I would agree that porting it would be in the best
> interests of the project. I can certainly bump the priority if you think
> you won't be able to help out.
>
>
> I don't have much short-term interest in regular LTO, my main concern is
> getting ThinLTO (and its API) "right".
> LTO is largely a subset of ThinLTO in term of what needs to be handled,
> and seems far less involved IMO.
>

Maybe there was a miscommunication. The new API would be used for both
regular LTO and ThinLTO. When I wrote *LTOCodeGenerator I meant
either LTOCodeGenerator or ThinLTOCodeGenerator.

-- 
-- 
Peter
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160512/703104c3/attachment.html>