[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:
> > > evgeny777 wrote:
> > > > 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`.
> > > This case is only handling global variables (getGVarSummary looks only for a summary of that type, and there is a cast below), so I'm not sure I understand how this part of the code relates to the case where the alias is dead and the aliasee is live. Can you clarify?
> > > 
> > > If the aliasee (the GVar) is dead, it seems like we could clear the read only flag during the thin link and get the same behavior as today.
> > > 
> > > Sorry if I am missing something. Can you give me a more concrete example of where/how this would go wrong for aliases?
> > Consider the following example:
> > 
> > foo.ll
> > ```
> > @g = global i32 42, align 4
> > @g.alias = weak alias i32, i32* @g
> > ```
> > 
> > main.ll
> > ```
> > @g = external global i32
> > 
> > define i32 @main() {
> >   %v = load i32, i32* @g
> >   ret i32 %v
> > }
> > ```
> > 
> > Assume we link `main.ll` and `foo.ll` in ThinLTO mode. We have global variable `@g` and it's alias `@g.alias`, the latter is obviously dead. Still it doesn't mean we can't internalize aliasee (`@g`), does it?
> > Assume we link main.ll and foo.ll in ThinLTO mode. We have global variable @g and it's alias @g.alias, the latter is obviously dead. Still it doesn't mean we can't internalize aliasee (@g), does it?
> 
> I'm confused though at how this relates to my suggestion. Here you are checking whether the global variable, i.e. @g, is live or not. In this example it is live, so presumably you will go ahead and mark it for internalization here since it is also read only. If you move the liveness check into the thin link (propagateConstants), presumably the same thing would happen - it is live, so it can stay read only, and this code would still mark it for internalization.
Ah, got it now. Ok, I'll try this out


https://reviews.llvm.org/D49362