[GlobalsAA] Restrict ModRef result if any internal method has its address taken.

[asbirlea]

Summary

If there are any internal methods whose address was taken, conclude there is nothing known in relation of any other internal method and a global.

[asbirlea]

Set KnowNothing.

[nlopes]

LGTM!

[theraven]

I think this version of the test case is slightly oversimplified and it would be valid for an optimisation to remove the check. If there is any address-taken function that may have escaped, then it is not okay to assume that @llvm.objc.autoreleasePoolPop will definitely not write to it, but in this case there are no stores to an internal global and so GVN is at liberty to eliminate it entirely. The fact that it doesn't is a missed optimisation caused by not having sufficient knowledge of @llvm.objc.autoreleasePoolPop.

In the original version of the test case, the miscompile comes from the fact that a -dealloc method stores to deallocCalled. The -dealloc method is registered with the runtime and is in a data structure that visible to the runtime.

[asbirlea]

I updated the patch to be more generic, while still addressing this usecase.
The condition is: if there are any internal methods whose address is taken but for which we do not conclude that they do not modify globals, then cannot conclude lower than ModRef for any other (LocalLinkageCall, GlobalValue).
Tests included.

[asbirlea]

Remove missed comment.

[theraven]

This comment was removed by theraven.

[jdoerfert]

Do I understand correctly that this going to be conservative in the case the internal function with its address taken is readonly/none?
Either way, could we have a test for that to make sure we see the "good" behavior or a FIXME in the code and test explaining what is needed?

llvm/include/llvm/Analysis/GlobalsModRef.h

class GlobalsAAResult : public AAResultBase<GlobalsAAResult> {
   class FunctionInfo;
 
   const DataLayout &DL;
   std::function<const TargetLibraryInfo &(Function &F)> GetTLI;
 
   /// The globals that do not have their addresses taken.
   SmallPtrSet<const GlobalValue *, 8> NonAddressTakenGlobals;
 
+  /// Are there functions with local linkage that may modify globals.
+  bool UnknownFunctionsWithLocalLinkage = false;
+
   /// IndirectGlobals - The memory pointed to by this global is known to be
   /// 'owned' by the global.
   SmallPtrSet<const GlobalValue *, 8> IndirectGlobals;
 
   /// AllocsForIndirectGlobals - If an instruction allocates memory for an
   /// indirect global, this map indicates which one.
   DenseMap<const Value *, const GlobalValue *> AllocsForIndirectGlobals;
 

llvm/lib/Analysis/GlobalsModRef.cpp

 
 /// AnalyzeGlobals - Scan through the users of all of the internal
 /// GlobalValue's in the program.  If none of them have their "address taken"
 /// (really, their address passed to something nontrivial), record this fact,
 /// and record the functions that they are used directly in.
 void GlobalsAAResult::AnalyzeGlobals(Module &M) {
   SmallPtrSet<Function *, 32> TrackedFunctions;
   for (Function &F : M)
-    if (F.hasLocalLinkage())
+    if (F.hasLocalLinkage()) {
       if (!AnalyzeUsesOfPointer(&F)) {
         // Remember that we are tracking this global.
         NonAddressTakenGlobals.insert(&F);
         TrackedFunctions.insert(&F);
         Handles.emplace_front(*this, &F);
         Handles.front().I = Handles.begin();
         ++NumNonAddrTakenFunctions;
       }
+      else
+        UnknownFunctionsWithLocalLinkage = true;
+    }
 
   SmallPtrSet<Function *, 16> Readers, Writers;
   for (GlobalVariable &GV : M.globals())
     if (GV.hasLocalLinkage()) {
       if (!AnalyzeUsesOfPointer(&GV, &Readers,
                                 GV.isConstant() ? nullptr : &Writers)) {
         // Remember that we are tracking this global, and the mod/ref fns
         NonAddressTakenGlobals.insert(&GV);
for (unsigned i = 0, e = SCC.size(); i != e && !KnowNothing; ++i) {
         } else if (F->onlyReadsMemory()) {
           FI.addModRefInfo(ModRefInfo::Ref);
           if (!F->isIntrinsic() && !F->onlyAccessesArgMemory())
             // This function might call back into the module and read a global -
             // consider every global as possibly being read by this function.
             FI.setMayReadAnyGlobal();
         } else {
           FI.addModRefInfo(ModRefInfo::ModRef);
-          // Can't say anything useful unless it's an intrinsic - they don't
-          // read or write global variables of the kind considered here.
-          KnowNothing = !F->isIntrinsic();
+          if (!F->onlyAccessesArgMemory())
+            FI.setMayReadAnyGlobal();
+          if (!F->isIntrinsic()) {
+            KnowNothing = true;
+            break;
+          }
         }
         continue;
       }
 
       for (CallGraphNode::iterator CI = SCC[i]->begin(), E = SCC[i]->end();
            CI != E && !KnowNothing; ++CI)
         if (Function *Callee = CI->second->getFunction()) {
           if (FunctionInfo *CalleeFI = getFunctionInfo(Callee)) {
ModRefInfo GlobalsAAResult::getModRefInfo(const CallBase *Call,
                                           const MemoryLocation &Loc,
                                           AAQueryInfo &AAQI) {
   ModRefInfo Known = ModRefInfo::ModRef;
 
   // If we are asking for mod/ref info of a direct call with a pointer to a
   // global we are tracking, return information if we have it.
   if (const GlobalValue *GV =
           dyn_cast<GlobalValue>(GetUnderlyingObject(Loc.Ptr, DL)))
-    if (GV->hasLocalLinkage())
+    // If GV is internal to this IR and there is no function with local linkage
+    // that has had their address taken, keep looking for a tighter ModRefInfo.
+    if (GV->hasLocalLinkage() && !UnknownFunctionsWithLocalLinkage)
       if (const Function *F = Call->getCalledFunction())
         if (NonAddressTakenGlobals.count(GV))
           if (const FunctionInfo *FI = getFunctionInfo(F))
             Known = unionModRef(FI->getModRefInfoForGlobal(*GV),
                                 getModRefInfoForArgument(Call, GV, AAQI));
 
   if (!isModOrRefSet(Known))
     return ModRefInfo::NoModRef; // No need to query other mod/ref analyses

llvm/test/Analysis/GlobalsModRef/intrinsic_addressnottaken1.ll

+; RUN: opt -globals-aa -gvn -S < %s | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@deallocCalled = internal global i8 0, align 1
+
+; CHECK-LABEL: @main()
+define dso_local i32 @main() {
+entry:
+  %tmp0 = call i8* @llvm.objc.autoreleasePoolPush() #1
+  %tmp6 = load i8, i8* @deallocCalled, align 1
+  %tobool = icmp ne i8 %tmp6, 0
+  br i1 %tobool, label %if.else, label %if.end
+
+if.else:                                          ; preds = %entry
+  call void @__assert_fail() #0
+  unreachable
+
+; CHECK-LABEL: if.end:
+; CHECK-NEXT: call void @llvm.objc.autoreleasePoolPop
+; CHECK-NOT: load i8, i8* @deallocCalled
+if.end:                                           ; preds = %entry
+  call void @llvm.objc.autoreleasePoolPop(i8* %tmp0)
+  %tmp7 = load i8, i8* @deallocCalled, align 1
+  %tobool3 = icmp ne i8 %tmp7, 0
+  br i1 %tobool3, label %if.end6, label %if.else5
+
+if.else5:                                         ; preds = %if.end
+  call void @__assert_fail() #0
+  unreachable
+
+if.end6:                                          ; preds = %if.end
+  store i8 0, i8* @deallocCalled, align 1
+  ret i32 0
+}
+
+declare i8* @llvm.objc.autoreleasePoolPush() #1
+declare void @llvm.objc.autoreleasePoolPop(i8*) #1
+declare dso_local void @__assert_fail() #0
+
+attributes #0 = { noreturn nounwind }
+attributes #1 = { nounwind }
+

llvm/test/Analysis/GlobalsModRef/intrinsic_addressnottaken2.ll

+; RUN: opt -globals-aa -gvn -S < %s | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@deallocCalled = internal global i8 0, align 1
+
+define internal i8* @_i_Associated__dealloc() {
+entry:
+  store i8 1, i8* @deallocCalled, align 1
+  ret i8* null
+}
+
+; CHECK-LABEL: @main()
+define dso_local i32 @main() {
+entry:
+  %tmp0 = call i8* @llvm.objc.autoreleasePoolPush() #1
+  %tmp6 = load i8, i8* @deallocCalled, align 1
+  %tobool = icmp ne i8 %tmp6, 0
+  br i1 %tobool, label %if.else, label %if.end
+
+if.else:                                          ; preds = %entry
+  call void @__assert_fail() #0
+  unreachable
+
+; CHECK-LABEL: if.end:
+; CHECK-NEXT: call void @llvm.objc.autoreleasePoolPop
+; CHECK-NOT: load i8, i8* @deallocCalled
+if.end:                                           ; preds = %entry
+  call void @llvm.objc.autoreleasePoolPop(i8* %tmp0)
+  %tmp7 = load i8, i8* @deallocCalled, align 1
+  %tobool3 = icmp ne i8 %tmp7, 0
+  br i1 %tobool3, label %if.end6, label %if.else5
+
+if.else5:                                         ; preds = %if.end
+  call void @__assert_fail() #0
+  unreachable
+
+if.end6:                                          ; preds = %if.end
+  store i8 0, i8* @deallocCalled, align 1
+  ret i32 0
+}
+
+declare i8* @llvm.objc.autoreleasePoolPush() #1
+declare void @llvm.objc.autoreleasePoolPop(i8*) #1
+declare dso_local void @__assert_fail() #0
+
+attributes #0 = { noreturn nounwind }
+attributes #1 = { nounwind }
+

llvm/test/Analysis/GlobalsModRef/intrinsic_addresstaken.ll

+; RUN: opt -globals-aa -gvn -S < %s | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@deallocCalled = internal global i8 0, align 1
+@.objc_method_list = internal global { i8* ()* } { i8* ()*  @_i_Associated__dealloc }, align 8
+@._OBJC_CLASS_Associated = global { i8* } { i8* bitcast ({ i8* ()* }* @.objc_method_list to i8*) }, align 8
+@._OBJC_INIT_CLASS_Associated = global { i8* }* @._OBJC_CLASS_Associated
[e-mail] = appending global [1 x i8*] [i8* bitcast ({ i8* }** @._OBJC_INIT_CLASS_Associated to i8*)]
+
+define internal i8* @_i_Associated__dealloc() {
+entry:
+  store i8 1, i8* @deallocCalled, align 1
+  ret i8* null
+}
+
+; CHECK-LABEL: @main()
+define dso_local i32 @main() {
+entry:
+  %tmp0 = call i8* @llvm.objc.autoreleasePoolPush() #1
+  %tmp6 = load i8, i8* @deallocCalled, align 1
+  %tobool = icmp ne i8 %tmp6, 0
+  br i1 %tobool, label %if.else, label %if.end
+
+if.else:                                          ; preds = %entry
+  call void @__assert_fail() #0
+  unreachable
+
+; CHECK-LABEL: if.end:
+; CHECK-NEXT: call void @llvm.objc.autoreleasePoolPop
+; CHECK-NEXT: load i8, i8* @deallocCalled
+if.end:                                           ; preds = %entry
+  call void @llvm.objc.autoreleasePoolPop(i8* %tmp0)
+  %tmp7 = load i8, i8* @deallocCalled, align 1
+  %tobool3 = icmp ne i8 %tmp7, 0
+  br i1 %tobool3, label %if.end6, label %if.else5
+
+if.else5:                                         ; preds = %if.end
+  call void @__assert_fail() #0
+  unreachable
+
+if.end6:                                          ; preds = %if.end
+  store i8 0, i8* @deallocCalled, align 1
+  ret i32 0
+}
+
+declare i8* @llvm.objc.autoreleasePoolPush() #1
+declare void @llvm.objc.autoreleasePoolPop(i8*) #1
+declare dso_local void @__assert_fail() #0
+
+attributes #0 = { noreturn nounwind }
+attributes #1 = { nounwind }