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

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 added inline comments.


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:144
+      if (!S->isLive()) {
+        // Dead global vars can't be internalized. Ignore aliases here,
+        // because we don't import them (yet) and dead alias doesn't
----------------
tejohnson wrote:
> evgeny777 wrote:
> > tejohnson wrote:
> > > Please update the comment to say why they can't be internalized. We drop dead symbols anyway in the backend, but I guess the issue is that once it is internalized the GUID changes and we can't find the summary in dropDeadSymbols?
> > This was originally intended to prevent internalization of non-prevailing defs. See `test/LTO/Resolution/X86/not-prevailing-variables.ll`
> Right but I'm trying to get to the *why* of that no longer working if you didn't do the liveness check here. As noted in that test case, we should drop the definition of that non-prevailing var2. I'm speculating that this would stop working if you marked var2 as readOnly since it would get internalized which would mean that dropDeadSymbols would compute a different GUID for the now-internalized variable which in turn would mean that we can't find the summary to see that it is dead.
Well, here is what I find out:

- dropDeadSymbols is invoked right after promotion and before internalization of immutable GVs, which happens much later (after import is finished). I rechecked `not-prevailing-variables.ll` test case and it works perfectly, even if liveness check is removed. Small modification to `internalizeImmutableGVs` is still required: we need to ignore declarations, not assert on them.

- If we remove liveness check then `funcimport.ll` test case will fail. This happens because `dropDeadSymbols` checks for `ModuleSummaryIndex::isGlobalValueLive` which can return true even if `isLive()` returns false. In particular this happens when all symbols in the index are dead. 

That said I suggest to:
- remove liveness check from propagateConstants
- revert changes to llvm-link.cpp
- update the failed test cases


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:145
+        // Dead global vars can't be internalized. Ignore aliases here,
+        // because we don't import them (yet) and dead alias doesn't
+        // mean aliasee is also dead.
----------------
tejohnson wrote:
> evgeny777 wrote:
> > tejohnson wrote:
> > > If/when we start importing aliases to global vars, would that affect this code? I still think we wouldn't want to update the readonly bit on the aliasee if alias is dead. (We shouldn't do any importing of dead symbols in any case - only live roots are added to the import worklist.)
> > > If/when we start importing aliases to global vars, would that affect this code
> > 
> > I would simply move liveness check to `processGlobalsForThinLTO` where it originally was. This will also allow eliminating the calls to `propagateConstants` from llvm-link.cpp. Of course `computeImportForReferencedGlobals` must be modified to support aliases
> But I don't understand why that would require a change to any of this handling? I.e. if an alias is dead, we should never even encounter it in computeImportForReferencedGlobals, since we only compute importing starting from live roots, and if the alias is reached from a live root then by definition it can't be dead.
If we remove liveness check (see above) then, I guess, we can simply add required functionality to `computeImportForReferencedGlobals`


https://reviews.llvm.org/D49362