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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson 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
----------------
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.


================
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.
----------------
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.


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:155
+      // the same memory location and any modification to alias means aliasee is
+      // is also modified.
+      if (auto *GVS = dyn_cast<GlobalVarSummary>(S->getBaseObject()))
----------------
evgeny777 wrote:
> tejohnson wrote:
> > Comment is still a little vague. Specifically, from what I understand:
> > - global variable can't be marked read only if it is not eligible to import since we need to ensure that all external references get a local (imported) copy.
> > - a global variable can't be marked read only if it or any alias (since alias points to the same memory) are preserved or notEligibleToImport, since either of those means there could be writes that are not visible (because preserved means it could have external to DSO writes, and notEligibleToImport means it could have writes via inline assembly leading it to be in the @llvm.*used).
> > 
> > Can you update the comment to note the above specifics (adjusted if I am missing something)? Will avoid future head scratching when I or someone else looks at this again down the road.
> > Can you update the comment to note the above specifics
> 
> Can I simply quote you in comments (as your understanding is 100% correct)? :)
Sure =)


https://reviews.llvm.org/D49362