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

Wed Nov 14 14:23:17 PST 2018

On Wed, 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?
>

Yes, it the hash value of  the gvar summary for gCrashRecoveryEnabled
should be read only in the clang-func-mapping case and not in the libclang
case. Since it is imported into ParseAST in both cases, it should affect
the hash computation. E.g. it is imported, so should be in the ImportList
passed to computeCacheKey for ParseAst.o, which should in turn call
AddUsedThings for every summary in the ImportList (which hashes that
summary's read only bit). But clearly something is not working properly
here...

> After importing, I see:
>
> libclang ParseAST
> @_ZL21gCrashRecoveryEnabled.llvm.16761009941167954495 =
> available_externally hidden unnamed_addr global i1 false, align 1
>

This makes sense, since with libclang it is modified i.e. the readonly bit
is not set in its summary (so it is promoted due to export and cannot be
internalized).

> clang-func-mapping ParseAST
> @_ZL21gCrashRecoveryEnabled.llvm.16761009941167954495 = internal
> unnamed_addr global i1 false, align 1 #0
> attributes #0 = { "thinlto-internalize" }
>

This makes sense too, since in this case the variable is not modified
anywhere. So it can be internalized after importing. The question is why is
it getting an undefined ref, since it has a local copy?

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

Since the imported var is available_externally and not marked constant, the
optimizer should treat the value appropriate (e.g. not constant prop).

Teresa

> 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 |
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181114/d9b94f1b/attachment.html>