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

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 added a comment.

> In fact I think that making this change to shouldPromoteLocalToGlobal on the importing side to get the linkage type from the index would work today

Yep, this seems obvious and the very first version of this patch uses this kind of approach. Unfortunately it didn't work quite well for me. Modifying linkage in the index will work fine on promotion and
internalization phases. However it's the import phase where all bad things happen. When performing import ThinLTO loads source module and does specific modifications in it given list of globals to import
Those modifications includes renaming globals to avoid naming conflicts and setting linkage of imported entities to `available_externally`. After that IRMover performs all the dirty work.

The main problem in this scheme is handling read only external globals. We can't set internal linkage for them in `renameModuleForThinLTO`, because IRMover will fail to link variable definition to 
it's external declaration. These two variables will be considered different:

  



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:283
+    /// Indicates that global isn't modified by any live function
+    unsigned Constant : 1;
+
----------------
tejohnson wrote:
> Maybe have this flag just on the GlobalVarSummary? Not needed on the function summary, and presumably the alias summary could just look through to the base object summary.
Makes sense, I'll give it a try


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:530
+  // non-volatile load instructions.
+  unsigned ImmutableRefCnt;
+
----------------
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


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:148
+  // Step 3: for each global variable check if it is still constant. If it isn't
+  // then all its refs are not constant either.
+  DenseSet<GlobalValueSummary *> MutableGVSet;
----------------
tejohnson wrote:
> Why? The refs on a global variable are those referenced where it is defined/initialized. Presumably if it is written later it doesn't affect those references. 
> 
> Hmm, what if we store the address of another static global var into a pointer global variable, and then write through that pointer. How is that case handled by the patch? The load will be to the pointer, so presumably it will not be mutable here. I.e.:
> 
> static int A = 1;
> static int *B = &A;
> void foo() {
>    *B = 0;
> }
> 
> In this case, there will be a load of B in foo, so B is constant and not mutable. I might be missing how this is handled, but I don't see where/how A will be marked mutable. I think you might need to be conservative and treat all refs on other global variables as potentially written?
> 
> If the address was taken in a function I think you would be ok since it would be referenced by a non-load. 
Yep, I also stepped on this recently :). By now I'm considering all non-instruction refs to be non-constant


https://reviews.llvm.org/D49362