[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:
> 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`


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


================
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()))
----------------
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)? :)


https://reviews.llvm.org/D49362