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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson 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");
----------------
evgeny777 wrote:
> 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`
>> 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)?

Right, note I suggested doing it in the thin link. Similar to my comment in computeVariableSummary where I suggested checking the import eligibility during the thin link: "Alternatively, rather than trying to detect here when it can't be imported and initializing to not read only, during the thin link importing can you clear the read only flag on any variable with a reference that we aren't able to import? "

>> Also, why is the isLive check needed?
> To prevent internalization of non-prevailing external globals. See LTO/Resolution/X86/not-prevailing-variables.ll

Won't we eliminate these non-live variables in any case (regardless of whether we internalize)? Or will internalization mess that up? In any case, it's probably better if this is also checked during the thin link (e.g. propagateConstants), and just clear the read only flag if we shouldn't internalize in the backend.


https://reviews.llvm.org/D49362