[llvm] r232351 - [objc-arc] Make the ARC optimizer more conservative by forcing it to be non-safe in both direction, but mitigate the problem by noting that we just care if there was a further use.
Michael Gottesman
mgottesman at apple.com
Mon Mar 16 00:02:36 PDT 2015
Author: mgottesman
Date: Mon Mar 16 02:02:36 2015
New Revision: 232351
URL: http://llvm.org/viewvc/llvm-project?rev=232351&view=rev
Log:
[objc-arc] Make the ARC optimizer more conservative by forcing it to be non-safe in both direction, but mitigate the problem by noting that we just care if there was a further use.
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>
Modified:
llvm/trunk/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
llvm/trunk/lib/Transforms/ObjCARC/PtrState.cpp
llvm/trunk/test/Transforms/ObjCARC/basic.ll
llvm/trunk/test/Transforms/ObjCARC/nested.ll
Modified: llvm/trunk/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/ObjCARC/ObjCARCOpts.cpp?rev=232351&r1=232350&r2=232351&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/ObjCARC/ObjCARCOpts.cpp (original)
+++ llvm/trunk/lib/Transforms/ObjCARC/ObjCARCOpts.cpp Mon Mar 16 02:02:36 2015
@@ -1617,11 +1617,8 @@ bool ObjCARCOpt::PairUpRetainsAndRelease
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();
Modified: llvm/trunk/lib/Transforms/ObjCARC/PtrState.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/ObjCARC/PtrState.cpp?rev=232351&r1=232350&r2=232351&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/ObjCARC/PtrState.cpp (original)
+++ llvm/trunk/lib/Transforms/ObjCARC/PtrState.cpp Mon Mar 16 02:02:36 2015
@@ -221,7 +221,6 @@ bool BottomUpPtrState::HandlePotentialAl
return false;
DEBUG(dbgs() << "CanAlterRefCount: Seq: " << Seq << "; " << *Ptr << "\n");
- ClearKnownPositiveRefCount();
switch (Seq) {
case S_Use:
SetSeq(S_CanRelease);
Modified: llvm/trunk/test/Transforms/ObjCARC/basic.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/ObjCARC/basic.ll?rev=232351&r1=232350&r2=232351&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/ObjCARC/basic.ll (original)
+++ llvm/trunk/test/Transforms/ObjCARC/basic.ll Mon Mar 16 02:02:36 2015
@@ -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:
; 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:
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:
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
}
Modified: llvm/trunk/test/Transforms/ObjCARC/nested.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/ObjCARC/nested.ll?rev=232351&r1=232350&r2=232351&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/ObjCARC/nested.ll (original)
+++ llvm/trunk/test/Transforms/ObjCARC/nested.ll Mon Mar 16 02:02:36 2015
@@ -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:
More information about the llvm-commits
mailing list