[objc-arc] Make the ARC optimizer more conservative by forcing it to be non-safe...

The problem here is the infamous one direction known safe. I was
hesitant to turn it off before b/c of the potential for regressions
without an actual bug from users hitting the problem. This is that bug ;
).

The main performance impact of having known safe in both directions is
that often times it is very difficult to find two releases without a use
in-between them since we are so conservative with determining potential
uses. The one direction known safe gets around that problem by taking
advantage of many situations where we have two retains in a row,
allowing us to avoid that problem. That being said, the one direction
known safe is unsafe. Consider the following situation:

retain(x)
retain(x)
call(x)
call(x)
release(x)

Then we know the following about the reference count of x:

// rc(x) == N (for some N).
retain(x)
// rc(x) == N+1
retain(x)
// rc(x) == N+2
call A(x)
call B(x)
// rc(x) >= 1 (since we can not release a deallocated pointer).
release(x)
// rc(x) >= 0

That is all the information that we can know statically. That means that
we know that A(x), B(x) together can release (x) at most N+1 times. Lets
say that we remove the inner retain, release pair.

// rc(x) == N (for some N).
retain(x)
// rc(x) == N+1
call A(x)
call B(x)
// rc(x) >= 1
release(x)
// rc(x) >= 0

We knew before that A(x), B(x) could release x up to N+1 times meaning
that rc(x) may be zero at the release(x). That is not safe. On the other
hand, consider the following situation where we have a must use of
release(x) that x must be kept alive for after the release(x)**. Then we
know that:

// rc(x) == N (for some N).
retain(x)
// rc(x) == N+1
retain(x)
// rc(x) == N+2
call A(x)
call B(x)
// rc(x) >= 2 (since we know that we are going to release x and that that release can not be the last use of x).
release(x)
// rc(x) >= 1 (since we can not deallocate the pointer since we have a must use after x).
…
// rc(x) >= 1
use(x)

Thus we know that statically the calls to A(x), B(x) can together only
release rc(x) N times. Thus if we remove the inner retain, release pair:

// rc(x) == N (for some N).
retain(x)
// rc(x) == N+1
call A(x)
call B(x)
// rc(x) >= 1
…
// rc(x) >= 1
use(x)

We are still safe unless in the final … there are unbalanced retains,
releases which would have caused the program to blow up anyways even
before optimization occurred. The simplest form of must use is an
additional release that has not been paired up with any retain (if we
had paired the release with a retain and removed it we would not have
the additional use). This fits nicely into the ARC framework since
basically what you do is say that given any nested releases regardless
of what is in between, the inner release is known safe. This enables us to get
back the lost performance.

<rdar://problem/19023795>

llvm-svn: 232351

diff --git a/llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp b/llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
index 31c5666..a517ffb 100644 (file)
--- a/llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
+++ b/llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
@@ -1617,11 +1617,8 @@ bool ObjCARCOpt::PairUpRetainsAndReleases(
     if (NewRetains.empty()) break;
   }
 
-  // If the pointer is known incremented in 1 direction and we do not have
-  // MultipleOwners, we can safely remove the retain/releases. Otherwise we need
-  // to be known safe in both directions.
-  bool UnconditionallySafe = (KnownSafeTD && KnownSafeBU) ||
-    ((KnownSafeTD || KnownSafeBU) && !MultipleOwners);
+  // We can only remove pointers if we are known safe in both directions.
+  bool UnconditionallySafe = KnownSafeTD && KnownSafeBU;
   if (UnconditionallySafe) {
     RetainsToMove.ReverseInsertPts.clear();
     ReleasesToMove.ReverseInsertPts.clear();

diff --git a/llvm/lib/Transforms/ObjCARC/PtrState.cpp b/llvm/lib/Transforms/ObjCARC/PtrState.cpp
index d360179..3581896 100644 (file)
--- a/llvm/lib/Transforms/ObjCARC/PtrState.cpp
+++ b/llvm/lib/Transforms/ObjCARC/PtrState.cpp
@@ -221,7 +221,6 @@ bool BottomUpPtrState::HandlePotentialAlterRefCount(Instruction *Inst,
     return false;
 
   DEBUG(dbgs() << "CanAlterRefCount: Seq: " << Seq << "; " << *Ptr << "\n");
-  ClearKnownPositiveRefCount();
   switch (Seq) {
   case S_Use:
     SetSeq(S_CanRelease);

diff --git a/llvm/test/Transforms/ObjCARC/basic.ll b/llvm/test/Transforms/ObjCARC/basic.ll
index 8bb0244..dc5f0b3 100644 (file)
--- a/llvm/test/Transforms/ObjCARC/basic.ll
+++ b/llvm/test/Transforms/ObjCARC/basic.ll
@@ -900,13 +900,14 @@ entry:
   ret i8* %x
 }
 
-; Trivial retain,release pair with intervening call, but it's dominated
-; by another retain - delete!
+; We can not delete this retain, release since we do not have a post-dominating
+; use of the release.
 
 ; CHECK-LABEL: define void @test12(
 ; CHECK-NEXT: entry:
 ; CHECK-NEXT: @objc_retain(i8* %x)
-; CHECK-NOT: @objc_
+; CHECK-NEXT: @objc_retain
+; CHECK: @objc_release
 ; CHECK: }
 define void @test12(i8* %x, i64 %n) {
 entry:
@@ -942,6 +943,8 @@ entry:
 ; CHECK-NEXT: @objc_retain(i8* %x)
 ; CHECK-NEXT: @use_pointer
 ; CHECK-NEXT: @use_pointer
+; CHECK-NEXT: @use_pointer
+; CHECK-NEXT: @objc_release
 ; CHECK-NEXT: ret void
 ; CHECK-NEXT: }
 define void @test13b(i8* %x, i64 %n) {
@@ -951,6 +954,8 @@ entry:
   call void @use_pointer(i8* %x)
   call void @use_pointer(i8* %x)
   call void @objc_release(i8* %x) nounwind
+  call void @use_pointer(i8* %x)
+  call void @objc_release(i8* %x) nounwind
   ret void
 }
 
@@ -984,6 +989,8 @@ entry:
 ; CHECK-NEXT: @objc_autoreleasePoolPush
 ; CHECK-NEXT: @use_pointer
 ; CHECK-NEXT: @use_pointer
+; CHECK-NEXT: @use_pointer
+; CHECK-NEXT: @objc_release
 ; CHECK-NEXT: ret void
 ; CHECK-NEXT: }
 define void @test13d(i8* %x, i64 %n) {
@@ -994,17 +1001,22 @@ entry:
   call void @use_pointer(i8* %x)
   call void @use_pointer(i8* %x)
   call void @objc_release(i8* %x) nounwind
+  call void @use_pointer(i8* %x)
+  call void @objc_release(i8* %x) nounwind
   ret void
 }
 
-; Trivial retain,release pair with intervening call, but it's post-dominated
-; by another release - delete!
+; Trivial retain,release pair with intervening call, and it's post-dominated by
+; another release. But it is not known safe in the top down direction. We can
+; not eliminate it.
 
 ; CHECK-LABEL: define void @test14(
 ; CHECK-NEXT: entry:
+; CHECK-NEXT: @objc_retain
 ; CHECK-NEXT: @use_pointer
 ; CHECK-NEXT: @use_pointer
 ; CHECK-NEXT: @objc_release
+; CHECK-NEXT: @objc_release
 ; CHECK-NEXT: ret void
 ; CHECK-NEXT: }
 define void @test14(i8* %x, i64 %n) {
@@ -1073,6 +1085,9 @@ entry:
 ; CHECK-LABEL: define void @test16a(
 ; CHECK: @objc_retain(i8* %x)
 ; CHECK-NOT: @objc
+; CHECK: purple:
+; CHECK: @use_pointer
+; CHECK: @objc_release
 ; CHECK: }
 define void @test16a(i1 %a, i1 %b, i8* %x) {
 entry:
@@ -1101,12 +1116,18 @@ blue:
   br label %purple
 
 purple:
+  call void @use_pointer(i8* %x)
+  call void @objc_release(i8* %x) nounwind
   ret void
 }
 
 ; CHECK-LABEL: define void @test16b(
 ; CHECK: @objc_retain(i8* %x)
 ; CHECK-NOT: @objc
+; CHECK: purple:
+; CHECK-NEXT: @use_pointer
+; CHECK-NEXT: @use_pointer
+; CHECK-NEXT: @objc_release
 ; CHECK: }
 define void @test16b(i1 %a, i1 %b, i8* %x) {
 entry:
@@ -1135,12 +1156,18 @@ blue:
   br label %purple
 
 purple:
+  call void @use_pointer(i8* %x)
+  call void @use_pointer(i8* %x)
+  call void @objc_release(i8* %x) nounwind
   ret void
 }
 
 ; CHECK-LABEL: define void @test16c(
 ; CHECK: @objc_retain(i8* %x)
 ; CHECK-NOT: @objc
+; CHECK: purple:
+; CHECK: @use_pointer
+; CHECK: @objc_release
 ; CHECK: }
 define void @test16c(i1 %a, i1 %b, i8* %x) {
 entry:
@@ -1169,12 +1196,14 @@ blue:
   br label %purple
 
 purple:
+  call void @use_pointer(i8* %x)
+  call void @objc_release(i8* %x) nounwind, !clang.imprecise_release !0
   ret void
 }
 
 ; CHECK-LABEL: define void @test16d(
 ; CHECK: @objc_retain(i8* %x)
-; CHECK-NOT: @objc
+; CHECK: @objc
 ; CHECK: }
 define void @test16d(i1 %a, i1 %b, i8* %x) {
 entry:
@@ -1206,44 +1235,6 @@ purple:
   ret void
 }
 
-
-; Retain+release pairs in diamonds, all post-dominated by a release.
-
-; CHECK-LABEL: define void @test17(
-; CHECK-NOT: @objc_
-; CHECK: purple:
-; CHECK: @objc_release
-; CHECK: }
-define void @test17(i1 %a, i1 %b, i8* %x) {
-entry:
-  br i1 %a, label %red, label %orange
-
-red:
-  call i8* @objc_retain(i8* %x) nounwind
-  br label %yellow
-
-orange:
-  call i8* @objc_retain(i8* %x) nounwind
-  br label %yellow
-
-yellow:
-  call void @use_pointer(i8* %x)
-  call void @use_pointer(i8* %x)
-  br i1 %b, label %green, label %blue
-
-green:
-  call void @objc_release(i8* %x) nounwind
-  br label %purple
-
-blue:
-  call void @objc_release(i8* %x) nounwind
-  br label %purple
-
-purple:
-  call void @objc_release(i8* %x) nounwind
-  ret void
-}
-
 ; Delete no-ops.
 
 ; CHECK-LABEL: define void @test18(
@@ -1936,6 +1927,9 @@ exit:                                             ; preds = %loop
 ; CHECK-NEXT: call i8* @objc_autorelease(i8* %p)
 ; CHECK-NEXT: call void @use_pointer(i8* %p)
 ; CHECK-NEXT: call void @use_pointer(i8* %p)
+; CHECK-NEXT: call void @use_pointer(i8* %p)
+; CHECK-NEXT: call void @use_pointer(i8* %p)
+; CHECK-NEXT: call void @objc_release(i8* %p)
 ; CHECK-NEXT: ret void
 ; CHECK-NEXT: }
 define void @test42(i8* %p) {
@@ -1946,6 +1940,9 @@ entry:
   call void @use_pointer(i8* %p)
   call void @use_pointer(i8* %p)
   call void @objc_release(i8* %p)
+  call void @use_pointer(i8* %p)
+  call void @use_pointer(i8* %p)
+  call void @objc_release(i8* %p)
   ret void
 }
 
@@ -1985,6 +1982,8 @@ entry:
 ; CHECK-NEXT: call void @use_pointer(i8* %p)
 ; CHECK-NEXT: call void @use_pointer(i8* %p)
 ; CHECK-NEXT: call i8* @objc_autoreleasePoolPush()
+; CHECK-NEXT: call void @use_pointer(i8* %p)
+; CHECK-NEXT: call void @objc_release
 ; CHECK-NEXT: ret void
 ; CHECK-NEXT: }
 define void @test43b(i8* %p) {
@@ -1996,6 +1995,8 @@ entry:
   call void @use_pointer(i8* %p)
   call i8* @objc_autoreleasePoolPush()
   call void @objc_release(i8* %p)
+  call void @use_pointer(i8* %p)
+  call void @objc_release(i8* %p)
   ret void
 }
 
@@ -2260,15 +2261,16 @@ if.end:                                           ; preds = %entry, %if.then
   ret void
 }
 
-; When there are adjacent retain+release pairs, the first one is
-; known unnecessary because the presence of the second one means that
-; the first one won't be deleting the object.
+; When there are adjacent retain+release pairs, the first one is known
+; unnecessary because the presence of the second one means that the first one
+; won't be deleting the object.
 
 ; CHECK-LABEL:      define void @test57(
 ; CHECK-NEXT: entry:
+; CHECK-NEXT:   tail call i8* @objc_retain(i8* %x) [[NUW]]
 ; CHECK-NEXT:   call void @use_pointer(i8* %x)
 ; CHECK-NEXT:   call void @use_pointer(i8* %x)
-; CHECK-NEXT:   %0 = tail call i8* @objc_retain(i8* %x) [[NUW]]
+; CHECK-NEXT:   tail call i8* @objc_retain(i8* %x) [[NUW]]
 ; CHECK-NEXT:   call void @use_pointer(i8* %x)
 ; CHECK-NEXT:   call void @use_pointer(i8* %x)
 ; CHECK-NEXT:   call void @objc_release(i8* %x) [[NUW]]
@@ -2277,6 +2279,7 @@ if.end:                                           ; preds = %entry, %if.then
 define void @test57(i8* %x) nounwind {
 entry:
   call i8* @objc_retain(i8* %x) nounwind
+  call i8* @objc_retain(i8* %x) nounwind
   call void @use_pointer(i8* %x)
   call void @use_pointer(i8* %x)
   call void @objc_release(i8* %x) nounwind
@@ -2292,6 +2295,7 @@ entry:
 
 ; CHECK-LABEL:      define void @test58(
 ; CHECK-NEXT: entry:
+; CHECK-NEXT:   @objc_retain
 ; CHECK-NEXT:   call void @use_pointer(i8* %x)
 ; CHECK-NEXT:   call void @use_pointer(i8* %x)
 ; CHECK-NEXT:   ret void
@@ -2299,6 +2303,7 @@ entry:
 define void @test58(i8* %x) nounwind {
 entry:
   call i8* @objc_retain(i8* %x) nounwind
+  call i8* @objc_retain(i8* %x) nounwind
   call void @use_pointer(i8* %x)
   call void @use_pointer(i8* %x)
   call void @objc_release(i8* %x) nounwind
@@ -2353,16 +2358,16 @@ define void @test60a() {
 ; CHECK-LABEL: define void @test60b(
 ; CHECK: call i8* @objc_retain
 ; CHECK-NOT: call i8* @objc_retain
-; CHECK-NOT: call i8* @objc_rrelease
+; CHECK-NOT: call i8* @objc_release
 ; CHECK: }
 define void @test60b() {
   %t = load i8*, i8** @constptr
   %s = load i8*, i8** @something
-  call i8* @objc_retain(i8* %s)
-  call i8* @objc_retain(i8* %s)
+  call i8* @objc_retain(i8* %t)
+  call i8* @objc_retain(i8* %t)
   call void @callee()
-  call void @use_pointer(i8* %t)
-  call void @objc_release(i8* %s)
+  call void @use_pointer(i8* %s)
+  call void @objc_release(i8* %t)
   ret void
 }
 
@@ -2372,10 +2377,10 @@ define void @test60b() {
 define void @test60c() {
   %t = load i8*, i8** @constptr
   %s = load i8*, i8** @something
-  call i8* @objc_retain(i8* %s)
+  call i8* @objc_retain(i8* %t)
   call void @callee()
-  call void @use_pointer(i8* %t)
-  call void @objc_release(i8* %s), !clang.imprecise_release !0
+  call void @use_pointer(i8* %s)
+  call void @objc_release(i8* %t), !clang.imprecise_release !0
   ret void
 }
 

diff --git a/llvm/test/Transforms/ObjCARC/nested.ll b/llvm/test/Transforms/ObjCARC/nested.ll
index 5ef7d62..464426a 100644 (file)
--- a/llvm/test/Transforms/ObjCARC/nested.ll
+++ b/llvm/test/Transforms/ObjCARC/nested.ll
@@ -229,7 +229,6 @@ entry:
   %items.ptr = alloca [16 x i8*], align 8
   %call = call i8* @returner()
   %0 = call i8* @objc_retainAutoreleasedReturnValue(i8* %call) nounwind
-  call void @callee()
   %tmp = bitcast %struct.__objcFastEnumerationState* %state.ptr to i8*
   call void @llvm.memset.p0i8.i64(i8* %tmp, i8 0, i64 64, i32 8, i1 false)
   %1 = call i8* @objc_retain(i8* %0) nounwind
@@ -283,13 +282,12 @@ forcoll.empty:
   ret void
 }
 
-; TODO: Delete a nested retain+release pair.
-; The optimizer currently can't do this, because isn't isn't sophisticated enough in
-; reasnoning about nesting.
-
+; We handle this now due to the fact that a release just needs a post dominating
+; use.
+;
 ; CHECK-LABEL: define void @test6(
 ; CHECK: call i8* @objc_retain
-; CHECK: @objc_retain
+; CHECK-NOT: @objc_retain
 ; CHECK: }
 define void @test6() nounwind {
 entry: