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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:372
+  // Regular LTO module doesn't participate in ThinLTO import,
+  // so no reference from it can be read-only.
+  if (IsThinLTO)
----------------
suggest adding something like "since this would require importing variable as local copy"


================
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
----------------
Where is this being handled? I don't see any special handling of references from GlobalVars here.


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:149
+      // which have references.
+      if (S->notEligibleToImport() || !S->refs().empty() ||
+          GUIDPreservedSymbols.count(P.first))
----------------
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.


================
Comment at: tools/llvm-lto/llvm-lto.cpp:389
   }
+  CombinedIndex.propagateConstants({});
   std::error_code EC;
----------------
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).


https://reviews.llvm.org/D49362