[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:
> 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.
>  it's probably better if this is also checked during the thin link

We should be able to handle case when alias is dead and aliasee is live (and vice versa). I don't see how this is possible to do with just manipulating `ReadOnly` attribute because we don't have it in `AliasSummary`.


https://reviews.llvm.org/D49362