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

Tue Nov 6 10:43:51 PST 2018

tejohnson 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
----------------
evgeny777 wrote:
> 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
Can you update the comment here to note that this is done by only marking refs from functions as read only when building the module summary during the analysis phase? Also suggest adding some checking here under NDEBUG that if VI is read only that S is not a global var summary. Will help prevent drift between the comments here and any changes to the module summary analysis phase later.

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

I'm not sure why we couldn't import and make a local copy of the aliasee when an alias to it is preserved and/or not eligible to import - assuming the reference is to the aliasee not the alias. In fact, we currently will never import a reference to an alias of a global var AFAICT [1].

[1] computeImportForReferencedGlobals only handles refs from functions directly to GlobalVarSummary objects - it doesn't call getBaseObject on the refs. So I don't believe we will even consider importing an alias to a global var. If that is ever supported in the future, we would presumably want to handle it like we do importing of an alias to a function, which is to import as a local copy [2]. In that case, we'd presumably want to ensure that the alias was importable, that the aliasee is importable, and that the aliasee is read only. However I don't think that requires marking the aliasee as not read only if the alias is not importable.

[1] As an aside, looking at function importing, it will look through to the base object (in selectCallee) *before* checking the notEligibleToImport flag. So I don't think having an alias marked as notEligibleToImport will actually prevent it from being imported! I think this works currently since AFAICT the only time we will mark an alias summary as notEligibleToImport is when it is a local that can't be promoted/renamed, and we only import aliases to functions as a local copy, which doesn't require promotion/renaming of the alias in the original module. This seems a little dicey to me, if we ever decide to mark an alias summary as notEligibleToImport for some other reason it might cause failures...not related at all to this patch, just dumping my thoughts here as a note to self...

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

Ok. In any case, extracting the importability check into a helper called both here and in the importer will keep these checks consistent (I'd prefer the interposability to be checked redundantly over having the checks for importability be different).

================
Comment at: tools/llvm-lto/llvm-lto.cpp:389
   }
+  CombinedIndex.propagateConstants({});
   std::error_code EC;
----------------
evgeny777 wrote:
> 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?
Ah, that test uses some legacy functionality in llvm-link to test importing. We've already had to work around the fact that it isn't doing a true thin link before importing, with a hack to mark all variables as exported (in linkFiles). To fix this you should be able to move this call to propagateConstants() onto the index after we load it in importFunctions (in llvm-link.cpp). 

The code here is just building the original combined index which in most cases is used to drive thin link optimizations via llvm-lto. Probably these old tests that use llvm-link to test importing should be migrated to llvm-lto and the import handling in llvm-link should be ripped out. I'll try to get to this today or tomorrow, but in the meantime, you should be able to work around in llvm-link itself as noted above.

https://reviews.llvm.org/D49362