From: Michael Gottesman Date: Fri, 20 Feb 2015 00:02:49 +0000 (+0000) Subject: [objc-arc-contract] We can not move retains over instructions which can not conservat... X-Git-Tag: llvmorg-3.7.0-rc1~11419 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=0fc2accb587b2748fac55663f077babdae68861f;p=platform%2Fupstream%2Fllvm.git [objc-arc-contract] We can not move retains over instructions which can not conservatively be proven to not decrement the retain's RCIdentity. I also cleaned up the code to make it more understandable for mere mortals. llvm-svn: 229937 --- diff --git a/llvm/lib/Transforms/ObjCARC/DependencyAnalysis.h b/llvm/lib/Transforms/ObjCARC/DependencyAnalysis.h index e5f9db9..8e042d4 100644 --- a/llvm/lib/Transforms/ObjCARC/DependencyAnalysis.h +++ b/llvm/lib/Transforms/ObjCARC/DependencyAnalysis.h @@ -76,6 +76,12 @@ bool CanAlterRefCount(const Instruction *Inst, const Value *Ptr, bool CanDecrementRefCount(const Instruction *Inst, const Value *Ptr, ProvenanceAnalysis &PA, ARCInstKind Class); +static inline bool CanDecrementRefCount(const Instruction *Inst, + const Value *Ptr, + ProvenanceAnalysis &PA) { + return CanDecrementRefCount(Inst, Ptr, PA, GetARCInstKind(Inst)); +} + } // namespace objcarc } // namespace llvm diff --git a/llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp b/llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp index f413c64..6473d3a 100644 --- a/llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp +++ b/llvm/lib/Transforms/ObjCARC/ObjCARCContract.cpp @@ -191,67 +191,174 @@ bool ObjCARCContract::contractAutorelease( return true; } -/// Attempt to merge an objc_release with a store, load, and objc_retain to form -/// an objc_storeStrong. This can be a little tricky because the instructions -/// don't always appear in order, and there may be unrelated intervening -/// instructions. -void -ObjCARCContract:: -tryToContractReleaseIntoStoreStrong(Instruction *Release, inst_iterator &Iter) { - LoadInst *Load = dyn_cast(GetArgRCIdentityRoot(Release)); - if (!Load || !Load->isSimple()) return; - - // For now, require everything to be in one basic block. - BasicBlock *BB = Release->getParent(); - if (Load->getParent() != BB) return; - - // Walk down to find the store and the release, which may be in either order. - BasicBlock::iterator I = Load, End = BB->end(); - ++I; - AliasAnalysis::Location Loc = AA->getLocation(Load); +static StoreInst *findSafeStoreForStoreStrongContraction(LoadInst *Load, + Instruction *Release, + ProvenanceAnalysis &PA, + AliasAnalysis *AA) { StoreInst *Store = nullptr; bool SawRelease = false; - for (; !Store || !SawRelease; ++I) { - if (I == End) - return; - Instruction *Inst = I; + // Get the location associated with Load. + AliasAnalysis::Location Loc = AA->getLocation(Load); + + // Walk down to find the store and the release, which may be in either order. + for (auto I = std::next(BasicBlock::iterator(Load)), + E = Load->getParent()->end(); + I != E; ++I) { + // If we found the store we were looking for and saw the release, + // break. There is no more work to be done. + if (Store && SawRelease) + break; + + // Now we know that we have not seen either the store or the release. If I + // is the the release, mark that we saw the release and continue. + Instruction *Inst = &*I; if (Inst == Release) { SawRelease = true; continue; } + // Otherwise, we check if Inst is a "good" store. Grab the instruction class + // of Inst. ARCInstKind Class = GetBasicARCInstKind(Inst); - // Unrelated retains are harmless. + // If Inst is an unrelated retain, we don't care about it. + // + // TODO: This is one area where the optimization could be made more + // aggressive. if (IsRetain(Class)) continue; + // If we have seen the store, but not the release... if (Store) { - // The store is the point where we're going to put the objc_storeStrong, - // so make sure there are no uses after it. - if (CanUse(Inst, Load, PA, Class)) - return; - } else if (AA->getModRefInfo(Inst, Loc) & AliasAnalysis::Mod) { - // We are moving the load down to the store, so check for anything - // else which writes to the memory between the load and the store. - Store = dyn_cast(Inst); - if (!Store || !Store->isSimple()) return; - if (Store->getPointerOperand() != Loc.Ptr) return; + // We need to make sure that it is safe to move the release from its + // current position to the store. This implies proving that any + // instruction in between Store and the Release conservatively can not use + // the RCIdentityRoot of Release. If we can prove we can ignore Inst, so + // continue... + if (!CanUse(Inst, Load, PA, Class)) { + continue; + } + + // Otherwise, be conservative and return nullptr. + return nullptr; } + + // Ok, now we know we have not seen a store yet. See if Inst can write to + // our load location, if it can not, just ignore the instruction. + if (!(AA->getModRefInfo(Inst, Loc) & AliasAnalysis::Mod)) + continue; + + Store = dyn_cast(Inst); + + // If Inst can, then check if Inst is a simple store. If Inst is not a + // store or a store that is not simple, then we have some we do not + // understand writing to this memory implying we can not move the load + // over the write to any subsequent store that we may find. + if (!Store || !Store->isSimple()) + return nullptr; + + // Then make sure that the pointer we are storing to is Ptr. If so, we + // found our Store! + if (Store->getPointerOperand() == Loc.Ptr) + continue; + + // Otherwise, we have an unknown store to some other ptr that clobbers + // Loc.Ptr. Bail! + return nullptr; } - Value *New = GetRCIdentityRoot(Store->getValueOperand()); + // If we did not find the store or did not see the release, fail. + if (!Store || !SawRelease) + return nullptr; + + // We succeeded! + return Store; +} - // Walk up to find the retain. - I = Store; - BasicBlock::iterator Begin = BB->begin(); - while (I != Begin && GetBasicARCInstKind(I) != ARCInstKind::Retain) +static Instruction * +findRetainForStoreStrongContraction(Value *New, StoreInst *Store, + Instruction *Release, + ProvenanceAnalysis &PA) { + // Walk up from the Store to find the retain. + BasicBlock::iterator I = Store; + BasicBlock::iterator Begin = Store->getParent()->begin(); + while (I != Begin && GetBasicARCInstKind(I) != ARCInstKind::Retain) { + Instruction *Inst = &*I; + + // It is only safe to move the retain to the store if we can prove + // conservatively that nothing besides the release can decrement reference + // counts in between the retain and the store. + if (CanDecrementRefCount(Inst, New, PA) && Inst != Release) + return nullptr; --I; + } Instruction *Retain = I; if (GetBasicARCInstKind(Retain) != ARCInstKind::Retain) + return nullptr; + if (GetArgRCIdentityRoot(Retain) != New) + return nullptr; + return Retain; +} + +/// Attempt to merge an objc_release with a store, load, and objc_retain to form +/// an objc_storeStrong. An objc_storeStrong: +/// +/// objc_storeStrong(i8** %old_ptr, i8* new_value) +/// +/// is equivalent to the following IR sequence: +/// +/// ; Load old value. +/// %old_value = load i8** %old_ptr (1) +/// +/// ; Increment the new value and then release the old value. This must occur +/// ; in order in case old_value releases new_value in its destructor causing +/// ; us to potentially have a dangling ptr. +/// tail call i8* @objc_retain(i8* %new_value) (2) +/// tail call void @objc_release(i8* %old_value) (3) +/// +/// ; Store the new_value into old_ptr +/// store i8* %new_value, i8** %old_ptr (4) +/// +/// The safety of this optimization is based around the following +/// considerations: +/// +/// 1. We are forming the store strong at the store. Thus to perform this +/// optimization it must be safe to move the retain, load, and release to +/// (4). +/// 2. We need to make sure that any re-orderings of (1), (2), (3), (4) are +/// safe. +void ObjCARCContract::tryToContractReleaseIntoStoreStrong(Instruction *Release, + inst_iterator &Iter) { + // See if we are releasing something that we just loaded. + auto *Load = dyn_cast(GetArgRCIdentityRoot(Release)); + if (!Load || !Load->isSimple()) + return; + + // For now, require everything to be in one basic block. + BasicBlock *BB = Release->getParent(); + if (Load->getParent() != BB) + return; + + // First scan down the BB from Load, looking for a store of the RCIdentityRoot + // of Load's + StoreInst *Store = + findSafeStoreForStoreStrongContraction(Load, Release, PA, AA); + // If we fail, bail. + if (!Store) + return; + + // Then find what new_value's RCIdentity Root is. + Value *New = GetRCIdentityRoot(Store->getValueOperand()); + + // Then walk up the BB and look for a retain on New without any intervening + // instructions which conservatively might decrement ref counts. + Instruction *Retain = + findRetainForStoreStrongContraction(New, Store, Release, PA); + + // If we fail, bail. + if (!Retain) return; - if (GetArgRCIdentityRoot(Retain) != New) return; Changed = true; ++NumStoreStrongs; diff --git a/llvm/test/Transforms/ObjCARC/contract-storestrong.ll b/llvm/test/Transforms/ObjCARC/contract-storestrong.ll index f9308c8..c218e33 100644 --- a/llvm/test/Transforms/ObjCARC/contract-storestrong.ll +++ b/llvm/test/Transforms/ObjCARC/contract-storestrong.ll @@ -24,7 +24,7 @@ entry: ; Don't do this if the load is volatile. -; CHECK: define void @test1(i8* %p) { +; CHECK-LABEL: define void @test1(i8* %p) { ; CHECK-NEXT: entry: ; CHECK-NEXT: %0 = tail call i8* @objc_retain(i8* %p) [[NUW]] ; CHECK-NEXT: %tmp = load volatile i8** @x, align 8 @@ -43,7 +43,7 @@ entry: ; Don't do this if the store is volatile. -; CHECK: define void @test2(i8* %p) { +; CHECK-LABEL: define void @test2(i8* %p) { ; CHECK-NEXT: entry: ; CHECK-NEXT: %0 = tail call i8* @objc_retain(i8* %p) [[NUW]] ; CHECK-NEXT: %tmp = load i8** @x, align 8 @@ -63,15 +63,15 @@ entry: ; Don't do this if there's a use of the old pointer value between the store ; and the release. -; CHECK: define void @test3(i8* %newValue) { -; CHECK-NEXT: entry: -; CHECK-NEXT: %x0 = tail call i8* @objc_retain(i8* %newValue) [[NUW]] -; CHECK-NEXT: %x1 = load i8** @x, align 8 -; CHECK-NEXT: store i8* %x0, i8** @x, align 8 -; CHECK-NEXT: tail call void @use_pointer(i8* %x1), !clang.arc.no_objc_arc_exceptions !0 -; CHECK-NEXT: tail call void @objc_release(i8* %x1) [[NUW]], !clang.imprecise_release !0 -; CHECK-NEXT: ret void -; CHECK-NEXT: } +; CHECK-LABEL: define void @test3(i8* %newValue) { +; CHECK-NEXT: entry: +; CHECK-NEXT: %x0 = tail call i8* @objc_retain(i8* %newValue) [[NUW]] +; CHECK-NEXT: %x1 = load i8** @x, align 8 +; CHECK-NEXT: store i8* %x0, i8** @x, align 8 +; CHECK-NEXT: tail call void @use_pointer(i8* %x1), !clang.arc.no_objc_arc_exceptions !0 +; CHECK-NEXT: tail call void @objc_release(i8* %x1) [[NUW]], !clang.imprecise_release !0 +; CHECK-NEXT: ret void +; CHECK-NEXT: } define void @test3(i8* %newValue) { entry: %x0 = tail call i8* @objc_retain(i8* %newValue) nounwind @@ -84,15 +84,15 @@ entry: ; Like test3, but with an icmp use instead of a call, for good measure. -; CHECK: define i1 @test4(i8* %newValue, i8* %foo) { -; CHECK-NEXT: entry: -; CHECK-NEXT: %x0 = tail call i8* @objc_retain(i8* %newValue) [[NUW]] -; CHECK-NEXT: %x1 = load i8** @x, align 8 -; CHECK-NEXT: store i8* %x0, i8** @x, align 8 -; CHECK-NEXT: %t = icmp eq i8* %x1, %foo -; CHECK-NEXT: tail call void @objc_release(i8* %x1) [[NUW]], !clang.imprecise_release !0 -; CHECK-NEXT: ret i1 %t -; CHECK-NEXT: } +; CHECK-LABEL: define i1 @test4(i8* %newValue, i8* %foo) { +; CHECK-NEXT: entry: +; CHECK-NEXT: %x0 = tail call i8* @objc_retain(i8* %newValue) [[NUW]] +; CHECK-NEXT: %x1 = load i8** @x, align 8 +; CHECK-NEXT: store i8* %x0, i8** @x, align 8 +; CHECK-NEXT: %t = icmp eq i8* %x1, %foo +; CHECK-NEXT: tail call void @objc_release(i8* %x1) [[NUW]], !clang.imprecise_release !0 +; CHECK-NEXT: ret i1 %t +; CHECK-NEXT: } define i1 @test4(i8* %newValue, i8* %foo) { entry: %x0 = tail call i8* @objc_retain(i8* %newValue) nounwind @@ -105,7 +105,7 @@ entry: ; Do form an objc_storeStrong here, because the use is before the store. -; CHECK: define i1 @test5(i8* %newValue, i8* %foo) { +; CHECK-LABEL: define i1 @test5(i8* %newValue, i8* %foo) { ; CHECK: %t = icmp eq i8* %x1, %foo ; CHECK: tail call void @objc_storeStrong(i8** @x, i8* %newValue) [[NUW]] ; CHECK: } @@ -121,7 +121,7 @@ entry: ; Like test5, but the release is before the store. -; CHECK: define i1 @test6(i8* %newValue, i8* %foo) { +; CHECK-LABEL: define i1 @test6(i8* %newValue, i8* %foo) { ; CHECK: %t = icmp eq i8* %x1, %foo ; CHECK: tail call void @objc_storeStrong(i8** @x, i8* %newValue) [[NUW]] ; CHECK: } @@ -137,7 +137,7 @@ entry: ; Like test0, but there's no store, so don't form an objc_storeStrong. -; CHECK-LABEL: define void @test7( +; CHECK-LABEL: define void @test7( ; CHECK-NEXT: entry: ; CHECK-NEXT: %0 = tail call i8* @objc_retain(i8* %p) [[NUW]] ; CHECK-NEXT: %tmp = load i8** @x, align 8 @@ -154,7 +154,7 @@ entry: ; Like test0, but there's no retain, so don't form an objc_storeStrong. -; CHECK-LABEL: define void @test8( +; CHECK-LABEL: define void @test8( ; CHECK-NEXT: entry: ; CHECK-NEXT: %tmp = load i8** @x, align 8 ; CHECK-NEXT: store i8* %p, i8** @x, align 8 @@ -169,6 +169,54 @@ entry: ret void } +; Make sure that we properly handle release that *may* release our new +; value in between the retain and the store. We need to be sure that +; this we can safely move the retain to the store. This specific test +; makes sure that we properly handled a release of an unrelated +; pointer. +; +; CHECK-LABEL: define i1 @test9(i8* %newValue, i8* %foo, i8* %unrelated_ptr) { +; CHECK-NOT: objc_storeStrong +define i1 @test9(i8* %newValue, i8* %foo, i8* %unrelated_ptr) { +entry: + %x0 = tail call i8* @objc_retain(i8* %newValue) nounwind + tail call void @objc_release(i8* %unrelated_ptr) nounwind, !clang.imprecise_release !0 + %x1 = load i8** @x, align 8 + tail call void @objc_release(i8* %x1) nounwind, !clang.imprecise_release !0 + %t = icmp eq i8* %x1, %foo + store i8* %newValue, i8** @x, align 8 + ret i1 %t +} + +; Make sure that we don't perform the optimization when we just have a call. +; +; CHECK-LABEL: define i1 @test10(i8* %newValue, i8* %foo, i8* %unrelated_ptr) { +; CHECK-NOT: objc_storeStrong +define i1 @test10(i8* %newValue, i8* %foo, i8* %unrelated_ptr) { +entry: + %x0 = tail call i8* @objc_retain(i8* %newValue) nounwind + call void @use_pointer(i8* %unrelated_ptr) + %x1 = load i8** @x, align 8 + tail call void @objc_release(i8* %x1) nounwind, !clang.imprecise_release !0 + %t = icmp eq i8* %x1, %foo + store i8* %newValue, i8** @x, align 8 + ret i1 %t +} + +; Make sure we form the store strong if the use in between the retain +; and the store does not touch reference counts. +; CHECK-LABEL: define i1 @test11(i8* %newValue, i8* %foo, i8* %unrelated_ptr) { +; CHECK: objc_storeStrong +define i1 @test11(i8* %newValue, i8* %foo, i8* %unrelated_ptr) { +entry: + %x0 = tail call i8* @objc_retain(i8* %newValue) nounwind + %t = icmp eq i8* %newValue, %foo + %x1 = load i8** @x, align 8 + tail call void @objc_release(i8* %x1) nounwind, !clang.imprecise_release !0 + store i8* %newValue, i8** @x, align 8 + ret i1 %t +} + !0 = !{} ; CHECK: attributes [[NUW]] = { nounwind }