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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Great! I only had a chance to take a cursory look so far (attending the llvm dev meeting), but a few questions/comments based on a very quick look this morning.

One high level comment is that it would be better to mark the linkage type on the index during the thin link when we can internalize, rather than have that logic in the back end as in this patch:

You'd need to either add the comdat flag to the summary (like you mentioned is needed anyway for incremental builds), or better yet could you just conservatively mark the GlobalVarSummary as non-constant during the compile step module summary analysis (i.e. instead of initializing the Constant bit to false, init it to true for all global variables except those with comdats - then your thin link Step 1 would just set it to false if it's in the GUIDPreservedSymbols set and otherwise not change the flag). This should fix your incremental build issue too.

Then if the thin link step marks those that are constant with internal linkage in the index and applies that in the backend, it would be consistent with the way we do other summary based internalization.

The main thing that I think would need to be changed is in shouldPromoteLocalToGlobal for the importing side we would need to look at the linkage type in the index rather than assuming that anything that is imported must be promoted. (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, since presumably any imported locals are already marked as ExternalLinkage in the index since we currently promote them all.) For the exporting side we already set the linkage type based on what is set in the index, so that would presumably stay the same.

I may be missing something that makes this problematic though, let me know what you think.

A couple other comments below.



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:283
+    /// Indicates that global isn't modified by any live function
+    unsigned Constant : 1;
+
----------------
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.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:530
+  // non-volatile load instructions.
+  unsigned ImmutableRefCnt;
+
----------------
Think it would be clearer to have a ref edge type and include a flag there.


================
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;
----------------
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. 


https://reviews.llvm.org/D49362