From: Hal Finkel Date: Sat, 30 Aug 2014 12:48:33 +0000 (+0000) Subject: Fix AddAliasScopeMetadata to not add scopes when deriving from unknown pointers X-Git-Url: http://plrg.eecs.uci.edu/git/?a=commitdiff_plain;h=bd896c1b25395ab2d304229b311bda148d38b629;p=oota-llvm.git 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 d49dc82774e..833cc2d4d75 100644 --- 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(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(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(V) || isa(V) || + isa(V) || + isa(V) || isa(V); + if (!IsNonPtrConst && + !isIdentifiedFunctionLocal(const_cast(V))) { + UsesAliasingPtr = true; + if (!isa(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(I) || isa(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(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 df21fc880f7..d2404013d22 100644 --- 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}