<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Nov 14, 2018, at 2:23 PM, Teresa Johnson <<a href="mailto:tejohnson@google.com" class="">tejohnson@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Wed, Nov 14, 2018 at 1:11 PM Steven Wu <<a href="mailto:stevenwu@apple.com" class="">stevenwu@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word; line-break: after-white-space;" class=""><div class="">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:</div><div class=""><br class=""></div><div class="">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?</div></div></blockquote><div class=""><br class=""></div><div class="">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...</div><div class=""><br class=""></div><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word; line-break: after-white-space;" class=""><div class=""><br class=""></div><div class="">After importing, I see:</div><div class=""><br class=""></div><div class="">libclang ParseAST</div><div class="">@_ZL21gCrashRecoveryEnabled.llvm.16761009941167954495 = available_externally hidden unnamed_addr global i1 false, align 1</div></div></blockquote><div class=""><br class=""></div><div class="">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). </div></div></div></div></blockquote><div><br class=""></div><div>I guess it is unclear what available_externally linkage does for global variables. It is the linkage for C 'inline' functions, which means the function is either inline or external. Now we treat it just like external for global variables and ignoring the value when it is not a constant. Sounds like a reasonable interpretation just not sure if this is official.</div><div><br class=""></div><div>Steven</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><div class="gmail_quote"><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word; line-break: after-white-space;" class=""><div class=""><br class=""></div><div class="">clang-func-mapping ParseAST</div><div class="">@_ZL21gCrashRecoveryEnabled.llvm.16761009941167954495 = internal unnamed_addr global i1 false, align 1 #0</div><div class="">attributes #0 = { "thinlto-internalize" }</div></div></blockquote><div class=""><br class=""></div><div class="">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?</div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word; line-break: after-white-space;" class=""><div class=""><br class=""></div><div class="">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.</div></div></blockquote><div class=""><br class=""></div><div class="">Since the imported var is available_externally and not marked constant, the optimizer should treat the value appropriate (e.g. not constant prop). </div><div class=""><br class=""></div><div class="">Teresa</div><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word; line-break: after-white-space;" class=""><div class=""><br class=""></div><div class="">Steven<br class=""><div class=""><br class=""></div></div><div class=""><div class=""><div class=""><br class=""><blockquote type="cite" class=""><div class="">On Nov 14, 2018, at 9:57 AM, Teresa Johnson <<a href="mailto:tejohnson@google.com" target="_blank" class="">tejohnson@google.com</a>> wrote:</div><br class="m_6546176354043812613Apple-interchange-newline"><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; text-decoration: none;" class=""><div dir="ltr" class=""><br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Wed, Nov 14, 2018 at 9:42 AM Steven Wu <<a href="mailto:stevenwu@apple.com" target="_blank" class="">stevenwu@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div class="">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.<div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class="">Another note, it will be good to add assembly support for readonly bits. It will be easier to debug.</div></div></blockquote><div class=""><br class=""></div><div class="">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).</div><div class=""><br class=""></div><div class="">Teresa</div><div class=""><br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div class=""><div class=""><br class=""></div><div class="">Steven</div><div class=""><div class=""><br class=""><blockquote type="cite" class=""><div class="">On Nov 14, 2018, at 9:27 AM, Evgeny Leviant <<a href="mailto:eleviant@accesssoftek.com" target="_blank" class="">eleviant@accesssoftek.com</a>> wrote:</div><br class="m_6546176354043812613gmail-m_1897825770343646756Apple-interchange-newline"><div class=""><div style="margin-top: 0px; margin-bottom: 0px; font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 16px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; background-color: rgb(255, 255, 255); text-decoration: none;" class="">I couldn't reproduce this on Linux, now trying Mac OS X (will take the whole night as it is slow)<br class=""></div><div style="margin-top: 0px; margin-bottom: 0px; font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 16px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; background-color: rgb(255, 255, 255); text-decoration: none;" class="">Stewen, can you please send me CMake configuration string from your buildbot. It would be helpful <br class=""></div><div style="margin-top: 0px; margin-bottom: 0px; font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 16px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; background-color: rgb(255, 255, 255); text-decoration: none;" class="">if pending OS X build also succeed.<br class=""></div><div style="margin-top: 0px; margin-bottom: 0px; font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 16px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; background-color: rgb(255, 255, 255); text-decoration: none;" class=""><br class=""></div><div style="margin-top: 0px; margin-bottom: 0px; font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 16px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; background-color: rgb(255, 255, 255); text-decoration: none;" class="">What I'm doing is:<br class=""></div><div style="margin-top: 0px; margin-bottom: 0px; font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 16px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; background-color: rgb(255, 255, 255); text-decoration: none;" class="">- build LLVM/clang with thin LTO cache<br class=""></div><div style="margin-top: 0px; margin-bottom: 0px; font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 16px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; background-color: rgb(255, 255, 255); text-decoration: none;" class="">- When finished remove cache from disk, then remove and rebuild libclang.so(dylib)<br class=""></div><div style="margin-top: 0px; margin-bottom: 0px; font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 16px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; background-color: rgb(255, 255, 255); text-decoration: none;" class="">- Remove and rebuild clang-func-mapping<br class=""></div><div style="margin-top: 0px; margin-bottom: 0px; font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 16px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; background-color: rgb(255, 255, 255); text-decoration: none;" class=""><br class=""></div><div style="margin-top: 0px; margin-bottom: 0px; font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 16px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; background-color: rgb(255, 255, 255); text-decoration: none;" class="">Are those steps correct?<br class=""></div><div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 16px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; background-color: rgb(255, 255, 255); text-decoration: none; color: rgb(33, 33, 33);" class=""><div class=""><hr style="display: inline-block; width: 1010.38px;" class=""><div id="m_6546176354043812613gmail-m_1897825770343646756x_divRplyFwdMsg" dir="ltr" class=""><font face="Calibri, sans-serif" style="font-size: 11pt;" class=""><b class="">От:</b><span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span>Teresa Johnson via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank" class="">reviews@reviews.llvm.org</a>><br class=""><b class="">Отправлено:</b><span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span>14 ноября 2018 г. 19:00<br class=""><b class="">Кому:</b><span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span>Evgeny Leviant;<span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span><a href="mailto:tejohnson@google.com" target="_blank" class="">tejohnson@google.com</a>;<span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span><a href="mailto:joker.eph@gmail.com" target="_blank" class="">joker.eph@gmail.com</a><br class=""><b class="">Копия:</b><span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span><a href="mailto:trent.xin.tong@gmail.com" target="_blank" class="">trent.xin.tong@gmail.com</a>;<span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span><a href="mailto:arphaman@gmail.com" target="_blank" class="">arphaman@gmail.com</a>;<span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span><a href="mailto:dany.grumberg@gmail.com" target="_blank" class="">dany.grumberg@gmail.com</a>;<span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span><a href="mailto:jfbastien@apple.com" target="_blank" class="">jfbastien@apple.com</a>;<span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span><a href="mailto:anton@korobeynikov.info" target="_blank" class="">anton@korobeynikov.info</a>;<span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span><a href="mailto:llvm@inglorion.net" target="_blank" class="">llvm@inglorion.net</a>;<span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span><a href="mailto:eraman@google.com" target="_blank" class="">eraman@google.com</a>;<span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span><a href="mailto:stevenwu@apple.com" target="_blank" class="">stevenwu@apple.com</a>;<span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span><a href="mailto:dexonsmith@apple.com" target="_blank" class="">dexonsmith@apple.com</a>;<span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span><a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>; George Rimar; Igor Kudrin;<span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span><a href="mailto:jun.l@samsung.com" target="_blank" class="">jun.l@samsung.com</a><br class=""><b class="">Тема:</b><span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span>[PATCH] D49362: [ThinLTO] Internalize read only globals</font><div class=""> </div></div></div><font size="2" class=""><span style="font-size: 10pt;" class=""><div class="m_6546176354043812613gmail-m_1897825770343646756PlainText">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.<br class=""><br class="">tejohnson added a comment.<br class=""><br class="">In<span class="m_6546176354043812613gmail-m_1897825770343646756Apple-converted-space"> </span><a href="https://reviews.llvm.org/D49362#1297188" target="_blank" class="">https://reviews.llvm.org/D49362#1297188</a>, @steven_wu wrote:<br class=""><br class="">> I reverted this in r346768.<br class="">><br class="">> 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.<br class="">> libclang.dylib thinks the file scope variable gCrashRecoveryEnabled not read-only, so it promotes it.<br class="">> 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.<br class="">><br class="">> Let me know if you need more information.<br class=""><br class=""><br class="">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.<br class=""><br class="">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.<br class=""><br class=""><br class="">Repository:<br class=""> <span class="m_6546176354043812613Apple-converted-space"> </span>rL LLVM<br class=""><br class=""><a href="https://reviews.llvm.org/D49362" target="_blank" class="">https://reviews.llvm.org/D49362</a><br class=""><br class=""><br class=""><br class=""></div></span></font></div></div></blockquote></div><br class=""></div></div></blockquote></div><br clear="all" class=""><div class=""><br class=""></div>--<span class="m_6546176354043812613Apple-converted-space"> </span><br class=""><div dir="ltr" class="m_6546176354043812613gmail_signature"><div dir="ltr" class=""><div class=""><span style="font-family: Times; font-size: inherit;" class=""><table cellspacing="0" cellpadding="0" class=""><tbody class=""><tr style="color: rgb(85, 85, 85); font-family: sans-serif; font-size: small;" class=""><td nowrap="" style="border-top-width: 2px; border-top-style: solid; border-top-color: rgb(213, 15, 37);" class="">Teresa Johnson |</td><td nowrap="" style="border-top-width: 2px; border-top-style: solid; border-top-color: rgb(51, 105, 232);" class=""> Software Engineer |</td><td nowrap="" style="border-top-width: 2px; border-top-style: solid; border-top-color: rgb(0, 153, 57);" class=""> <a href="mailto:tejohnson@google.com" target="_blank" class="">tejohnson@google.com</a> |</td></tr></tbody></table></span></div></div></div></div></div></div></blockquote></div><br class=""></div></div></div></blockquote></div><br clear="all" class=""><div class=""><br class=""></div>--<span class="Apple-converted-space"> </span><br class=""><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr" class=""><div class=""><span style="font-family: Times; font-size: inherit;" class=""><table cellspacing="0" cellpadding="0" class=""><tbody class=""><tr style="color: rgb(85, 85, 85); font-family: sans-serif; font-size: small;" class=""><td nowrap="" style="border-top-width: 2px; border-top-style: solid; border-top-color: rgb(213, 15, 37);" class="">Teresa Johnson |</td><td nowrap="" style="border-top-width: 2px; border-top-style: solid; border-top-color: rgb(51, 105, 232);" class=""> Software Engineer |</td><td nowrap="" style="border-top-width: 2px; border-top-style: solid; border-top-color: rgb(0, 153, 57);" class=""> <a href="mailto:tejohnson@google.com" target="_blank" class="">tejohnson@google.com</a> |</td></tr></tbody></table></span></div></div></div></div></div></blockquote></div><br class=""></body></html>