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

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 added inline comments.


================
Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:231
+    if (auto *GVS = getGVarSummary(ImportIndex, &GV))
+      if (GVS->isLive() && GVS->isReadOnly() && !GVS->notEligibleToImport())
+        cast<GlobalVariable>(&GV)->addAttribute("thinlto-internalize");
----------------
tejohnson wrote:
> We should check notEligibleToImport during the thin link, along the lines of my comment in computeVariableSummary. Also, why is the isLive check needed?
> We should check notEligibleToImport during the thin link, along the lines of my comment in computeVariableSummary

This is problematic because `notEligibleToImport` is calculated after computing per-module summaries. How about doing this in `propagateConstants` (as we're going to switch to single index pass algorithm anyway)?

> Also, why is the isLive check needed?

To prevent internalization of non-prevailing external globals. See `LTO/Resolution/X86/not-prevailing-variables.ll`


https://reviews.llvm.org/D49362