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

Wed Nov 14 17:24:40 PST 2018

Both sets of intermediate files have test1.c files (neither has test2.c
files).

On Wed, Nov 14, 2018 at 4:34 PM Steven Wu <stevenwu at apple.com> wrote:

>
>
> 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.
>
>
> Steven
>
>
>
>>
>> Steven
>>
>>
>> On Nov 14, 2018, at 1:11 PM, Steven Wu <stevenwu at apple.com> wrote:
>>
>> I am making some low progress on reproducing it on trunk. It is not very
>> easy to get into this state but this is what I found right now:
>>
>> ParseAST always has readonly reference to gCrashRecoveryEnabled and in
>> both linkage unit, gCrashRecoveryEnabled is imported into ParseAST. The
>> difference is that for libclang, there is a different module which modifies
>> gCrashRecoveryEnabled. Maybe I am missing something but will there be any
>> difference in hash value for ParseAST?
>>
>> After importing, I see:
>>
>> libclang ParseAST
>> @_ZL21gCrashRecoveryEnabled.llvm.16761009941167954495 =
>> available_externally hidden unnamed_addr global i1 false, align 1
>>
>> clang-func-mapping ParseAST
>> @_ZL21gCrashRecoveryEnabled.llvm.16761009941167954495 = internal
>> unnamed_addr global i1 false, align 1 #0
>> attributes #0 = { "thinlto-internalize" }
>>
>> I am little suspecting about the libclang case. Is this semantically
>> correct if it is inlined? It will not observe the changes to the value if
>> modified.
>>
>> Steven
>>
>>
>> On Nov 14, 2018, at 9:57 AM, Teresa Johnson <tejohnson at google.com> wrote:
>>
>>
>>
>> On Wed, Nov 14, 2018 at 9:42 AM Steven Wu <stevenwu at apple.com> wrote:
>>
>>> For the stage 2 build, just
>>> use clang/cmake/caches/Apple-stage2-ThinLTO.cmake. I also throw a profile
>>> data in my build just to match our builder but I suspect that might not be
>>> needed. I will double check to confirm that.
>>>
>>> You also don't need to wait for the entire build to finish. Just run
>>> `ninja libclang.dylib && ninja clang-func-mapping` is consistently
>>> reproducing for me. Let me know if you need more help with this.
>>>
>>> Another note, it will be good to add assembly support for readonly bits.
>>> It will be easier to debug.
>>>
>>
>> Good point - I completely forgot about this when reviewing. Eugene - you
>> will need to change AsmWriter.cpp, LLToken.h, LLParser.cpp and LLLexer.cpp
>> (see the changes to those files in D53345 as an example, although that was
>> adding a new function flag, but you would want to do something similar for
>> the new gvar flag).
>>
>> Teresa
>>
>>
>>> Steven
>>>
>>> On Nov 14, 2018, at 9:27 AM, Evgeny Leviant <eleviant at accesssoftek.com>
>>> wrote:
>>>
>>> I couldn't reproduce this on Linux, now trying Mac OS X (will take the
>>> whole night as it is slow)
>>> Stewen, can you please send me CMake configuration string from your
>>> buildbot. It would be helpful
>>> if pending OS X build also succeed.
>>>
>>> What I'm doing is:
>>> - build LLVM/clang with thin LTO cache
>>> - When finished remove cache from disk, then remove and rebuild
>>> libclang.so(dylib)
>>> - Remove and rebuild clang-func-mapping
>>>
>>> Are those steps correct?
>>> ------------------------------
>>> *От:* Teresa Johnson via Phabricator <reviews at reviews.llvm.org>
>>> *Отправлено:* 14 ноября 2018 г. 19:00
>>> *Кому:* Evgeny Leviant; tejohnson at google.com; joker.eph at gmail.com
>>> *Копия:* trent.xin.tong at gmail.com; arphaman at gmail.com;
>>> dany.grumberg at gmail.com; jfbastien at apple.com; anton at korobeynikov.info;
>>> llvm at inglorion.net; eraman at google.com; stevenwu at apple.com;
>>> dexonsmith at apple.com; llvm-commits at lists.llvm.org; George Rimar; Igor
>>> Kudrin; jun.l at samsung.com
>>> *Тема:* [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.
>>>
>>> tejohnson added a comment.
>>>
>>> In https://reviews.llvm.org/D49362#1297188, @steven_wu wrote:
>>>
>>> > I reverted this in r346768.
>>> >
>>> > I think this is indeed a caching problem. There are some dylib/binary
>>> in clang projects that toggles whether to enable the recovery context but
>>> some does not. I can reproduce the issue by thin link libclang.dylib first,
>>> then link clang-func-mapping on Darwin.
>>> >  libclang.dylib thinks the file scope variable gCrashRecoveryEnabled
>>> not read-only, so it promotes it.
>>> >  clang-func-mapping thinks gCrashRecoveryEnabled read-only, so it
>>> internalize and constant propagate the variable but ParseAST.o in the cache
>>> is still expecting gCrashRecoveryEnabled to be available.
>>> >
>>> > Let me know if you need more information.
>>>
>>>
>>> Thinking through this example, I'm not sure what is going on. This patch
>>> did change the cache key computation to include the read only bit from the
>>> global var summary of anything defined or imported. Since
>>> gCrashRecoveryEnabled is a static in CrashRecoveryContext.cpp, it must have
>>> been imported into ParseAST.o, which means that the read only bit on the
>>> associated gvar summary should have been hashed into ParseAST.o's cache
>>> key. So presumably we shouldn't have had a cache hit for ParseAST.o (built
>>> when the read only bit is not set on gCrashRecoveryEnabled) when thin
>>> linking clang-func-mapping which has this bit set for that variable.
>>>
>>> Even if the variable was originally externally visible, in order for it
>>> to be marked read only by the thin link it would have had to have been
>>> imported at all use sites. Which means that any referencing module had to
>>> have it in the import set (and therefore would have hashed the read only
>>> bit) in the thin link where it was marked read only. So I am not sure
>>> offhand why we could ever share a referencing object between a link where
>>> it was read only and another link where it is not...   Hopefully Eugene can
>>> figure out what is going wrong here.
>>>
>>>
>>> Repository:
>>>   rL LLVM
>>>
>>> https://reviews.llvm.org/D49362
>>>
>>>
>>>
>>>
>>>
>>
>> --
>> 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/20181114/e3084e36/attachment.html>