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

[Steven Wu via llvm-commits]

> On Nov 16, 2018, at 2:38 AM, Evgeny Leviant <eleviant at accesssoftek.com> wrote:
> 
> See r347033. I'll create a follow-up path with assembly representation of ReadOnly attribute next week, if this one goes well.

Thanks. I ran some simple examples through the new version and it indeed fix the issue. I will keep an eye on our CI as well.

Steven

> 
> От: Teresa Johnson via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>>
> Отправлено: 16 ноября 2018 г. 3:28
> Кому: Evgeny Leviant; tejohnson at google.com <mailto:tejohnson at google.com>; joker.eph at gmail.com <mailto:joker.eph at gmail.com>
> Копия: trent.xin.tong at gmail.com <mailto:trent.xin.tong at gmail.com>; arphaman at gmail.com <mailto:arphaman at gmail.com>; dany.grumberg at gmail.com <mailto:dany.grumberg at gmail.com>; jfbastien at apple.com <mailto:jfbastien at apple.com>; anton at korobeynikov.info <mailto:anton at korobeynikov.info>; llvm at inglorion.net <mailto:llvm at inglorion.net>; eraman at google.com <mailto:eraman at google.com>; stevenwu at apple.com <mailto:stevenwu at apple.com>; dexonsmith at apple.com <mailto:dexonsmith at apple.com>; llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>; George Rimar; Igor Kudrin; jun.l at samsung.com <mailto: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.
> 
> I was trying this out internally and noticed a couple of minor things that can be fixed when you recommit the patch with the caching fix.
> 
> 
> 
> ================
> Comment at: llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp:1048
> +      continue;
> +    if (auto *GVar = dyn_cast<GlobalVariable>(&GV))
> +      if (GVar->hasAttribute("thinlto-internalize")) {
> ----------------
> This should always be true - globals() returns only GlobalVariables (you shouldn't even need to cast).
> 
> 
> ================
> Comment at: llvm/trunk/lib/Transforms/Utils/FunctionImportUtils.cpp:225
> +  // propagateConstants hasn't been run (may be because we're using
> +  // distriuted import. We can't internalize GV in such case.
> +  if (!GV.isDeclaration() && VI && ImportIndex.withGlobalValueDeadStripping()) {
> ----------------
> As we discussed earlier in the review thread, there should not be any issue with doing this for a distributed import (I just checked a small test case and confirmed it works fine). Please update the comment (it was only in certain testing contexts that you wouldn't have dead stripping at this point).
> 
> 
> Repository:
>   rL LLVM
> 
> https://reviews.llvm.org/D49362 <https://reviews.llvm.org/D49362>
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181116/030894a9/attachment-0001.html>