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

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:828
+    // If import is disabled we should drop read-only attribute
+    // from all summaries to prevent internalization.
+    for (auto &P : Index)
----------------
tejohnson wrote:
> evgeny777 wrote:
> > tejohnson wrote:
> > > Why is this necessary? Since internalizeImmutableGVs is only invoked from importFunctions(), presumably it doesn't have any effect anyway when not importing?
> > On -lto-O0 ThinLTO import is entirely disabled, so we have to prevent internalization of read-only stuff or we'll get link errors.
> Right but since the internalization is done via internalizeImmutableGVs which is called from importFunctions, if importing is disabled under -lto-O0 wouldn't the internalization not be done either?
Well, strictly speaking, import in the backend is not disabled: `-lto-O0` just prevents computation of import/export lists, leaving them empty. Current approach marks read-only GVs with `thinlto-internalize` attribute, relying on IRMover to copy them to all destination modules, so internalization happens in all modules using read-only GV (or shouldn't happen at all). If `IRMover` doesn't do anything (because import lists are empty) we'll internalize GV just in the source module, leaving all external declarations unsatisfied.


https://reviews.llvm.org/D49362