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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Looks really good - just one minor code change request, and a few comment changes, and one question.



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


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


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:149
+          GVS->setReadOnly(false);
+        // We don't examine referneces from dead objects
+        continue;
----------------
typo: s/referneces/references/


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


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:298
+      if (isa<GlobalVarSummary>(RefSummary.get()) &&
           !GlobalValue::isInterposableLinkage(RefSummary->linkage()) &&
+          canImportGlobalVar(RefSummary.get())) {
----------------
Please move this one into canImportGlobalVar. I'd rather check it redundantly in the readonly case (i.e. even though you are already checking when building the summary and initializing that bit), to keep the checking centralized and consistent between here and there.


https://reviews.llvm.org/D49362