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

[Teresa Johnson via llvm-commits]

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