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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: tools/llvm-lto/llvm-lto.cpp:389
   }
+  CombinedIndex.propagateConstants({});
   std::error_code EC;
----------------
tejohnson wrote:
> 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.
I tried migrating the llvm-link tests to llvm-lto but it turns out not to be straightforward for a couple of tests that are testing importing in the presence of old debug info, because we don't have a way other than with llvm-link to force the importing of a given function, and the bitcode being old means the index is old and we don't have importing flags set so importing is conservative. So I suggest going with the fix I suggested above for now.


https://reviews.llvm.org/D49362