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

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 added inline comments.


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:106
+  // read-only either. Note that:
+  // - All references from GlobalVarSummary are conservatively considered as
+  //   not read-only. Tracking them properly requires more complex analysis
----------------
tejohnson wrote:
> Where is this being handled? I don't see any special handling of references from GlobalVars here.
In ValueInfo constructor we have:
```
RefAndFlags.setInt(HaveGVs);
```
This means that `ReadOnly` bit is zero, unless `setReadOnly` is called


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:149
+      // which have references.
+      if (S->notEligibleToImport() || !S->refs().empty() ||
+          GUIDPreservedSymbols.count(P.first))
----------------
tejohnson wrote:
> Should this and the above block of code (looking for live values) only be done if S is a GlobalVariableSummary?
> 
> Also, can the conditions for importing be shared with computeImportForReferencedGlobals so that the conditions don't diverge? I.e. some kind of helper method to do the checking. Also I noticed that computeImportForReferencedGlobals also checks whether it is interposable.
> Should this and the above block of code (looking for live values) only be done if S is a GlobalVariableSummary?

If alias is either not eligible to import or is preserved we can't make a local copy of aliasee, can we? If we make this check only for GlobalVarSummary we can internalize it if its alias is preserved

> Also, can the conditions for importing be shared with computeImportForReferencedGlobals so that the conditions don't diverge?

Sounds reasonable.

> Also I noticed that computeImportForReferencedGlobals also checks whether it is interposable.

We don't set ReadOnly bit for GVs with interposable linkage. See `computeVariableSummary`


================
Comment at: tools/llvm-lto/llvm-lto.cpp:389
   }
+  CombinedIndex.propagateConstants({});
   std::error_code EC;
----------------
tejohnson wrote:
> Why this change? IIRC this is just to test the creation of the combined index, with no optimizations done on it. Can we test the constant propagation elsewhere where the various thinlto optimizations are tested (e.g. see the ThinLTOMode handling).
Unless this is done the patch breaks `Linker/funcimport.ll` test case due to internalization of dead GV. I'm not quite aware of the purpose of this code and how to fix it properly. Suggestions?


https://reviews.llvm.org/D49362