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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

I was trying this out internally and noticed a couple of minor things that can be fixed when you recommit the patch with the caching fix.



================
Comment at: llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp:1048
+      continue;
+    if (auto *GVar = dyn_cast<GlobalVariable>(&GV))
+      if (GVar->hasAttribute("thinlto-internalize")) {
----------------
This should always be true - globals() returns only GlobalVariables (you shouldn't even need to cast).


================
Comment at: llvm/trunk/lib/Transforms/Utils/FunctionImportUtils.cpp:225
+  // propagateConstants hasn't been run (may be because we're using
+  // distriuted import. We can't internalize GV in such case.
+  if (!GV.isDeclaration() && VI && ImportIndex.withGlobalValueDeadStripping()) {
----------------
As we discussed earlier in the review thread, there should not be any issue with doing this for a distributed import (I just checked a small test case and confirmed it works fine). Please update the comment (it was only in certain testing contexts that you wouldn't have dead stripping at this point).


Repository:
  rL LLVM

https://reviews.llvm.org/D49362