Fix AddAliasScopeMetadata to not add scopes when deriving from unknown pointers

The previous implementation of AddAliasScopeMetadata, which adds noalias
metadata to preserve noalias parameter attribute information when inlining had
a flaw: it would add alias.scope metadata to accesses which might have been
derived from pointers other than noalias function parameters. This was
incorrect because even some access known not to alias with all noalias function
parameters could easily alias with an access derived from some other pointer.
Instead, when deriving from some unknown pointer, we cannot add alias.scope
metadata at all. This fixes a miscompile of the test-suite's tramp3d-v4.
Furthermore, we cannot add alias.scope to functions unless we know they
access only argument-derived pointers (currently, we know this only for
memory intrinsics).

Also, we fix a theoretical problem with using the NoCapture attribute to skip
the capture check. This is incorrect (as explained in the comment added), but
would not matter in any code generated by Clang because we get only inferred
nocapture attributes in Clang-generated IR.

This functionality is not yet enabled by default.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@216818 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/Transforms/Utils/InlineFunction.cpp b/lib/Transforms/Utils/InlineFunction.cpp
index d49dc82774efefb07972a789b2be2d9cc04c8863..833cc2d4d757853f13518559d4ad35cc6f52a560 100644 (file)
--- a/lib/Transforms/Utils/InlineFunction.cpp
+++ b/lib/Transforms/Utils/InlineFunction.cpp
@@ -471,17 +471,17 @@ static void AddAliasScopeMetadata(CallSite CS, ValueToValueMapTy &VMap,
       else if (const AtomicRMWInst *RMWI = dyn_cast<AtomicRMWInst>(I))
         PtrArgs.push_back(RMWI->getPointerOperand());
       else if (ImmutableCallSite ICS = ImmutableCallSite(I)) {
-       // If we know that the call does not access memory, then we'll still
-       // know that about the inlined clone of this call site, and we don't
-       // need to add metadata.
+        // If we know that the call does not access memory, then we'll still
+        // know that about the inlined clone of this call site, and we don't
+        // need to add metadata.
         if (ICS.doesNotAccessMemory())
           continue;
 
         for (ImmutableCallSite::arg_iterator AI = ICS.arg_begin(),
              AE = ICS.arg_end(); AI != AE; ++AI)
-         // We need to check the underlying objects of all arguments, not just
-         // the pointer arguments, because we might be passing pointers as
-         // integers, etc.
+          // We need to check the underlying objects of all arguments, not just
+          // the pointer arguments, because we might be passing pointers as
+          // integers, etc.
           // FIXME: If we know that the call only accesses pointer arguments,
           // then we only need to check the pointer arguments.
           PtrArgs.push_back(*AI);
@@ -512,12 +512,23 @@ static void AddAliasScopeMetadata(CallSite CS, ValueToValueMapTy &VMap,
 
       // Figure out if we're derived from anything that is not a noalias
       // argument.
-      bool CanDeriveViaCapture = false;
-      for (const Value *V : ObjSet)
-        if (!isIdentifiedFunctionLocal(const_cast<Value*>(V))) {
-          CanDeriveViaCapture = true;
-          break;
+      bool CanDeriveViaCapture = false, UsesAliasingPtr = false;
+      for (const Value *V : ObjSet) {
+        // Is this value a constant that cannot be derived from any pointer
+        // value (we need to exclude constant expressions, for example, that
+        // are formed from arithmetic on global symbols).
+        bool IsNonPtrConst = isa<ConstantInt>(V) || isa<ConstantFP>(V) ||
+                             isa<ConstantPointerNull>(V) ||
+                             isa<ConstantDataVector>(V) || isa<UndefValue>(V);
+        if (!IsNonPtrConst &&
+            !isIdentifiedFunctionLocal(const_cast<Value*>(V))) {
+          UsesAliasingPtr = true;
+          if (!isa<Argument>(V)) {
+            CanDeriveViaCapture = true;
+            break;
+          }
         }
+      }
   
       // First, we want to figure out all of the sets with which we definitely
       // don't alias. Iterate over all noalias set, and add those for which:
@@ -526,7 +537,12 @@ static void AddAliasScopeMetadata(CallSite CS, ValueToValueMapTy &VMap,
       //   2. The noalias argument has not yet been captured.
       for (const Argument *A : NoAliasArgs) {
         if (!ObjSet.count(A) && (!CanDeriveViaCapture ||
-                                 A->hasNoCaptureAttr() ||
+                                 // It might be tempting to skip the
+                                 // PointerMayBeCapturedBefore check if
+                                 // A->hasNoCaptureAttr() is true, but this is
+                                 // incorrect because nocapture only guarantees
+                                 // that no copies outlive the function, not
+                                 // that the value cannot be locally captured.
                                  !PointerMayBeCapturedBefore(A,
                                    /* ReturnCaptures */ false,
                                    /* StoreCaptures */ false, I, &DT)))
@@ -537,22 +553,32 @@ static void AddAliasScopeMetadata(CallSite CS, ValueToValueMapTy &VMap,
         NI->setMetadata(LLVMContext::MD_noalias, MDNode::concatenate(
           NI->getMetadata(LLVMContext::MD_noalias),
             MDNode::get(CalledFunc->getContext(), NoAliases)));
+
       // Next, we want to figure out all of the sets to which we might belong.
-      // We might below to a set if:
-      //  1. The noalias argument is in the set of underlying objects
-      // or
-      //  2. There is some non-noalias argument in our list and the no-alias
-      //     argument has been captured.
-      
-      for (const Argument *A : NoAliasArgs) {
-        if (ObjSet.count(A) || (CanDeriveViaCapture &&
-                                PointerMayBeCapturedBefore(A,
-                                  /* ReturnCaptures */ false,
-                                  /* StoreCaptures */ false,
-                                  I, &DT)))
-          Scopes.push_back(NewScopes[A]);
+      // We might belong to a set if the noalias argument is in the set of
+      // underlying objects. If there is some non-noalias argument in our list
+      // of underlying objects, then we cannot add a scope because the fact
+      // that some access does not alias with any set of our noalias arguments
+      // cannot itself guarantee that it does not alias with this access
+      // (because there is some pointer of unknown origin involved and the
+      // other access might also depend on this pointer). We also cannot add
+      // scopes to arbitrary functions unless we know they don't access any
+      // non-parameter pointer-values.
+      bool CanAddScopes = !UsesAliasingPtr;
+      if (CanAddScopes && (isa<CallInst>(I) || isa<InvokeInst>(I))) {
+        // FIXME: We should have a way to access the
+        // IntrReadArgMem/IntrReadWriteArgMem properties of intrinsics, and we
+        // should have a way to determine that for regular functions too. For
+        // now, just do this for the memory intrinsics we understand.
+        CanAddScopes = isa<MemIntrinsic>(I);
       }
 
+      if (CanAddScopes)
+        for (const Argument *A : NoAliasArgs) {
+          if (ObjSet.count(A))
+            Scopes.push_back(NewScopes[A]);
+        }
+
       if (!Scopes.empty())
         NI->setMetadata(LLVMContext::MD_alias_scope, MDNode::concatenate(
           NI->getMetadata(LLVMContext::MD_alias_scope),

diff --git a/test/Transforms/Inline/noalias-calls.ll b/test/Transforms/Inline/noalias-calls.ll
index df21fc880f70aa09a4f71aa55c5fd123f4fd70de..d2404013d221175ee6b37a68f263f954655b1bc4 100644 (file)
--- a/test/Transforms/Inline/noalias-calls.ll
+++ b/test/Transforms/Inline/noalias-calls.ll
@@ -22,8 +22,8 @@ entry:
 
 ; CHECK: define void @foo(i8* nocapture %a, i8* nocapture readonly %c, i8* nocapture %b) #1 {
 ; CHECK: entry:
-; CHECK:   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %a, i8* %b, i64 16, i32 16, i1 false) #0, !alias.scope !0, !noalias !3
-; CHECK:   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %b, i8* %c, i64 16, i32 16, i1 false) #0, !alias.scope !3, !noalias !0
+; CHECK:   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %a, i8* %b, i64 16, i32 16, i1 false) #0, !noalias !0
+; CHECK:   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %b, i8* %c, i64 16, i32 16, i1 false) #0, !noalias !3
 ; CHECK:   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %a, i8* %c, i64 16, i32 16, i1 false) #0, !alias.scope !5
 ; CHECK:   call void @hey() #0, !noalias !5
 ; CHECK:   ret void
@@ -33,9 +33,9 @@ attributes #0 = { nounwind }
 attributes #1 = { nounwind uwtable }
 
 ; CHECK: !0 = metadata !{metadata !1}
-; CHECK: !1 = metadata !{metadata !1, metadata !2, metadata !"hello: %a"}
+; CHECK: !1 = metadata !{metadata !1, metadata !2, metadata !"hello: %c"}
 ; CHECK: !2 = metadata !{metadata !2, metadata !"hello"}
 ; CHECK: !3 = metadata !{metadata !4}
-; CHECK: !4 = metadata !{metadata !4, metadata !2, metadata !"hello: %c"}
-; CHECK: !5 = metadata !{metadata !1, metadata !4}
+; CHECK: !4 = metadata !{metadata !4, metadata !2, metadata !"hello: %a"}
+; CHECK: !5 = metadata !{metadata !4, metadata !1}