[GlobalsAA] Don't assume anything about functions that may be overridden

[jmolloy]

Summary

Weak linkage and friends allow a symbol to be overriden outside the
code generator's model, so GlobalsAA shouldn't assume that anything it
can compute about such a symbol is valid.

This should make GlobalsAA more conservative and fix the last problem seen in the wild.

Chandler, you had concerns about GlobalsAA's conservativeness. Do you think this is sufficient or do you still have worries?

[jmolloy]

Ping!

[chandlerc]

This is clearly correct. Please feel free to commit.

I want to record that we can probably do much better than this. Specifically, we should model external (or replaceable) functions as mod/ref-ing all external (or captured externally) globals, but not mod/ref-ing any non-externally-captured internal globals. Not sure where the best place is to record that? If you find a good place, add the comment and move on. =]

lib/Analysis/GlobalsModRef.cpp

 /// function.
 void GlobalsAAResult::AnalyzeCallGraph(CallGraph &CG, Module &M) {
   // We do a bottom-up SCC traversal of the call graph.  In other words, we
   // visit all callees before callers (leaf-first).
   for (scc_iterator<CallGraph *> I = scc_begin(&CG); !I.isAtEnd(); ++I) {
     const std::vector<CallGraphNode *> &SCC = *I;
     assert(!SCC.empty() && "SCC with no functions?");
 
-    if (!SCC[0]->getFunction()) {
-      // Calls externally - can't say anything useful.  Remove any existing
+    if (!SCC[0]->getFunction() || SCC[0]->getFunction()->mayBeOverridden()) {
+      // Calls externally or is weak - can't say anything useful. Remove any existing
       // function records (may have been created when scanning globals).
       for (auto *Node : SCC)
         FunctionInfos.erase(Node->getFunction());
       continue;
     }
 
     FunctionInfo &FI = FunctionInfos[SCC[0]->getFunction()];
     bool KnowNothing = false;

test/Analysis/GlobalsModRef/weak-interposition.ll

+; RUN: opt -S -O1 -enable-non-lto-gmr=true < %s | FileCheck %s
+
+@a = common global i32 0, align 4
+
+; @hook_me is weak, so GMR must not eliminate the reload of @a in @f,
+; even though @hook_me doesn't mod or ref @a.
+
+; Function Attrs: nounwind ssp uwtable
+define weak i32 @hook_me() {
+  ret i32 0
+}
+
+; Function Attrs: nounwind ssp uwtable
+define i32 @f() {
+  %1 = alloca i32, align 4
+  store i32 4, i32* @a, align 4
+  %2 = call i32 @hook_me()
+  ; CHECK: load i32, i32* @a, align 4
+  %3 = load i32, i32* @a, align 4
+  %4 = add nsw i32 %3, %2
+  store i32 %4, i32* @a, align 4
+  %5 = load i32, i32* %1
+  ret i32 %5
+}