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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D49362#1275404, @evgeny777 wrote:

> > I see, yes that explanation makes sense. I suppose on the importing side we could go back and update the linkage for imported variables based on the linkage in the index after the IRMover importing is complete for the module (since the linkage in the index is currently ignored on the importing side). On the exporting side it would get internalized as usual during the index-based internalization. This has the advantage of being more consistent with how we currently internalize based on thin link analysis. Would that not work as expected?
>
> It seems to me as significantly more complex approach. At the first glance, I'll have to:
>
> - Prevent promotion on thin link phase in both `LTO.cpp` and `ThinLTOCodeGenerator.cpp` for all read-only variables.
> - On promote phase check all read-only variables with local linkage preventing them from becoming `available_externally`. Non-local vars must not be touched (or IRMover will fail to link them to declarations), so we also need step 3:
> - After import phase we still need to analyze external read only globals, internalizing them when possible. As I'm supposed to check index instead of IR attributes I'm puzzled how to do this properly because of changing linkage to `available_externally` during import process (so GUID will obviously change also)
>
>   So compared to current implementation we:
> - add extra step (prevent promotion in index) and make handling of external globals signifcantly more complex.
> - must have all three steps consistent, which means spreading checks around the code. For instance, if variable is not eligible to import it (a) shouldn't be prevented from promotion, (b) should have `available_externally` linkage when being imported and (c) shouldn't be internalized if it has non-local linkage.


Ok, this does seem substantially simpler, so let's go ahead with your current approach. I made a few more comments below, will take another look after you've had a chance to update.



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:530
+  // non-volatile load instructions.
+  unsigned ImmutableRefCnt;
+
----------------
evgeny777 wrote:
> tejohnson wrote:
> > evgeny777 wrote:
> > > tejohnson wrote:
> > > > Think it would be clearer to have a ref edge type and include a flag there.
> > > What about memory usage? Also, like I said, we have all constant references grouped at the end of RefEdgeList, so this looks excessive
> > I have mixed feelings on this. With your current scheme you don't add increase the in-memory index size, but it is unfortunate to have the read only flag on all ValueInfo when it only applies to reference edges. Let me take a look at some of our large indexes today and see what the extra overhead would be to make this explicit on just the ref edges.
> I've already changed the way immutable references are stored. Now I'm using an extra flag in `ValueInfo` which supposedly has zero overhead. I'm still using immutable reference counter when store/load index to/from bitcode file as all read-only references are still grouped in the end of RefEdges array to save disk space.
ok let's go with this approach. If we need to add more info on the ref edges in the future, it can be revisited.


================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:650
+  computeDeadSymbolsWithConstProp(Index, GUIDPreservedSymbols, isPrevailing,
+                                  true);
 }
----------------
document constant parameter.


================
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)
----------------
evgeny777 wrote:
> 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.
Ah ok, I missed that lto-O0 was just skipping the index based computation of imports and not the backend importFunctions invocation.


================
Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:228
+  // attribute. We'll internalize them after import is finished
+  // See internalizeImmutableGVs.
+  if (!GV.isDeclaration())
----------------
Please expand on why the internalization of the read only variables needs to be done this way (i.e. in two steps).


================
Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:230
+  if (!GV.isDeclaration())
+    if (auto *GVS = getGVarSummary(ImportIndex, &GV))
+      if (GVS->isLive() && GVS->isReadOnly() && !GVS->notEligibleToImport())
----------------
This adds another index hash table lookup for GV, which we do just above here too. Can you lookup the ValueInfo for the GV once for both blocks of code?


================
Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:231
+    if (auto *GVS = getGVarSummary(ImportIndex, &GV))
+      if (GVS->isLive() && GVS->isReadOnly() && !GVS->notEligibleToImport())
+        cast<GlobalVariable>(&GV)->addAttribute("thinlto-internalize");
----------------
We should check notEligibleToImport during the thin link, along the lines of my comment in computeVariableSummary. Also, why is the isLive check needed?


https://reviews.llvm.org/D49362