[PATCH] D49362: [ThinLTO] Internalize read only globals

Thu Nov 15 06:55:33 PST 2018

On Thu, Nov 15, 2018 at 6:46 AM Evgeny Leviant <eleviant at accesssoftek.com>
wrote:

> Teresa, does it make sense to fix the issue and recommit or it's better to
> wait for your patch?
>

Go ahead and just fix in the old LTO API cache key computation so you can
recommit.
Thanks

> ------------------------------
> *От:* Teresa Johnson <tejohnson at google.com>
> *Отправлено:* 15 ноября 2018 г. 17:44
> *Кому:* Evgeny Leviant
> *Копия:* Steven Wu; Mehdi AMINI;
> reviews+D49362+public+6f0316d7b1456727 at reviews.llvm.org; Xin Tong; Alex
> L; Daniel Grumberg; jfbastien at apple.com; Anton Korobeynikov; Bob Haarman;
> Easwaran Raman; Duncan P. N. Exon Smith; llvm-commits; George Rimar; Igor
> Kudrin; jun.l at samsung.com
> *Тема:* Re: [PATCH] D49362: [ThinLTO] Internalize read only globals
>
>
>
> On Thu, Nov 15, 2018 at 6:35 AM Teresa Johnson <tejohnson at google.com>
> wrote:
>
>>
>>
>> On Thu, Nov 15, 2018 at 6:22 AM Evgeny Leviant <eleviant at accesssoftek.com>
>> wrote:
>>
>>> Thanks for making reproducer, Steven!
>>>
>>>
>>> It turned out cache key is computed differently by newer API (LTO.cpp)
>>> and legacy API (ThinLTOCodeGenerator).
>>>
>> Arg - such a simple reason! I didn't remember that they were using
>> separate code. Looking at what appears to be the cache key computation for
>> the old API (ModuleCacheEntry constructor), I don't see it hashing other
>> things we compute during the thin link beyond import/exports/resolved
>> linkage types. I.e. I don't see it hashing the isLive bit. So I'm surprised
>> we haven't hit an issue here before. Wonder if this code and
>> computeCacheKey can be refactored to do the same computation for most of
>> the info?? Maybe as a follow on patch. But I'm really curious how things
>> are working given e.g. the missing hash of the live bit etc.
>>
>
> Answering my own question: The isLive bit is only used downstream of the
> thin link in the CFI/WPD case, which aren't supported via the old LTO API.
> Ditto for the DSOLocal bit, which is only set in the new LTO API.
>
> Anyway, I think it would be good to refactor and call common code for the
> cache key computation, but this can be done as a follow on. Steven - is
> this something you would have time to do? I can do it, but can't test with
> ld64. If you want I can create a patch and you can test it there.
>
> Teresa
>
>
>>
>> I was always wondering what piece of software uses legacy API and finally
>>> found this out - it is Mac OS X linker ld64.
>>>
>>
>> Yep, along with at least one proprietary linker that I am aware of.
>>
>> Teresa
>>
>>>
>>> On Linux lld and gold plugin use newer API, so the bug can't be
>>> reproduced.
>>> ------------------------------
>>> *От:* stevenwu at apple.com <stevenwu at apple.com> от имени Steven Wu <
>>> stevenwu at apple.com>
>>> *Отправлено:* 15 ноября 2018 г. 3:34
>>> *Кому:* Teresa Johnson
>>> *Копия:* Evgeny Leviant; Mehdi AMINI;
>>> reviews+D49362+public+6f0316d7b1456727 at reviews.llvm.org; Xin Tong; Alex
>>> L; Daniel Grumberg; JF Bastien; Anton Korobeynikov; Bob Haarman; Easwaran
>>> Raman; Duncan Exon Smith; llvm-commits; George Rimar; Igor Kudrin;
>>> jun.l at samsung.com
>>> *Тема:* Re: [PATCH] D49362: [ThinLTO] Internalize read only globals
>>>
>>> CAUTION: This email originated from outside of the organization. Do not
>>> click links or open attachments unless you recognize the sender and know
>>> the content is safe.
>>>
>>>
>>> On Nov 14, 2018, at 4:28 PM, Teresa Johnson <tejohnson at google.com>
>>> wrote:
>>>
>>>
>>>
>>> On Wed, Nov 14, 2018 at 1:47 PM Steven Wu <stevenwu at apple.com> wrote:
>>>
>>>> Here is a small reduced test case using the same main.c and foo.c from
>>>> the review:
>>>> cat main.c
>>>>
>>>> int foo();
>>>> int main() {
>>>>   return foo();
>>>> }
>>>>
>>>>
>>>> cat foo.c
>>>>
>>>> #include <stdlib.h>
>>>> static int gFoo = 1;
>>>> int foo() {
>>>>   return gFoo;
>>>> }
>>>> void bar() {
>>>>   gFoo = rand();
>>>> }
>>>>
>>>>
>>>> cat test1.c
>>>>
>>>> int foo();
>>>> int test() {
>>>>   return foo();
>>>> }
>>>>
>>>>
>>>> cat test2.c
>>>>
>>>> int foo();
>>>> int bar();
>>>> int test() {
>>>>   return foo() + bar();
>>>> }
>>>>
>>>>
>>>> To reproduce:
>>>> $ clang -O3 -flto=thin -c main.c test1.c test2.c foo.c
>>>> $ clang -O3 -flto=thin -Wl,-cache_path_lto,lto.cache main.o test2.o
>>>> foo.o
>>>> $ clang -O3 -flto=thin -Wl,-cache_path_lto,lto.cache main.o test1.o
>>>> foo.o
>>>> Undefined symbols for architecture x86_64:
>>>>   "_gFoo.llvm.17695746433417383459", referenced from:
>>>>       _main in lto.o
>>>>
>>>
>>> I can't reproduce this, although I am using gold
>>> (with  -Wl,-plugin-opt,cache-dir=lto.cache) since I don't have ld64. I
>>> notice that test() is dead, so with gold at least it is removed completely.
>>>
>>> Do you have any temp files you can share (i.e. IR before and after
>>> importing)?
>>>
>>>
>>> Attach all the intermediate file. I haven't had time to look in detail
>>> yet.
>>>
>>>
>>
>> --
>> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
>>
>
>
> --
> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
>

-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181115/93292ae0/attachment.html>