Skip to content

Improve @guaranteed args handling in ARCSequenceOpts #33267

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions lib/SILOptimizer/ARC/RefCountState.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,14 @@ bool BottomUpRefCountState::handlePotentialGuaranteedUser(
if (!mayGuaranteedUseValue(PotentialGuaranteedUser, getRCRoot(), AA))
return false;

// If we can prove that the pointer we are tracking cannot be decremented,
// return. On return, BottomUpRefCountState::handlePotentialUser can correctly
// handle transition of refcount state. It transitions from a Decrement
// refcount state to a MighBeUsed refcount state
if (!mayDecrementRefCount(PotentialGuaranteedUser, getRCRoot(), AA)) {
return false;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more clear if the comment explained that by returning, we allow handlePotentialUser to treat this like a normal use and transition to MightBeUsed

It's really too bad that we need to check handlePotentialDecrement again after this, because it's expensive (and confusing). But these helpers are really contorted and I don't know how to fix it.

// Instructions that we do not recognize (and thus will not move) and that
// *must* use RCIdentity, implies we are always known safe as long as meet
// over all path constraints are satisfied.
Expand Down Expand Up @@ -816,6 +824,13 @@ bool TopDownRefCountState::handlePotentialGuaranteedUser(
if (!mayGuaranteedUseValue(PotentialGuaranteedUser, getRCRoot(), AA))
return false;

// If we can prove that the pointer we are tracking cannot be decremented,
// return. On return, TopDownRefCountState::handlePotentialUser can correctly
// handle transition of refcount state.
if (!mayDecrementRefCount(PotentialGuaranteedUser, getRCRoot(), AA)) {
return false;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same feedback on the comment applies here.

// Otherwise, update our step given that we have a potential decrement.
return handleGuaranteedUser(PotentialGuaranteedUser, getRCRoot(),
SetFactory, AA);
Expand Down
47 changes: 47 additions & 0 deletions test/SILOptimizer/arcsequenceopts.sil
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,53 @@ bb0(%0 : $*Builtin.Int32):
return %4: $()
}

struct Int {
var value : Builtin.Int64
}

sil @guaranteed_call : $@convention(thin) (@guaranteed <τ_0_0> { var τ_0_0 } <Builtin.Int64>) -> () {
bb0(%0 : $<τ_0_0> { var τ_0_0 } <Builtin.Int64>):
%1 = tuple ()
return %1 : $()
}

// CHECK_LABEL: sil hidden [noinline] @$test_guaranteed_call :
// CHECK: bb1{{.*}}:
// CHECK-NOT: strong_retain
// CHECK: apply
// CHECK-NOT: strong_release
// CHECK: bb2:
// CHECK_LABEL: } // end sil function '$test_guaranteed_call'
sil hidden [noinline] @$test_guaranteed_call : $@convention(thin) () -> () {
bb0:
%box = alloc_box $<τ_0_0> { var τ_0_0 } <Builtin.Int64>
%proj = project_box %box : $<τ_0_0> { var τ_0_0 } <Builtin.Int64>, 0
%0 = integer_literal $Builtin.Int64, 1
%1 = integer_literal $Builtin.Int64, 100
%funcref = function_ref @guaranteed_call : $@convention(thin) (@guaranteed <τ_0_0> { var τ_0_0 } <Builtin.Int64>) -> ()
%3 = struct $Int (%0 : $Builtin.Int64)
%6 = integer_literal $Builtin.Int1, -1
br bb1(%0 : $Builtin.Int64)

bb1(%8 : $Builtin.Int64):
%9 = builtin "sadd_with_overflow_Int64"(%8 : $Builtin.Int64, %0 : $Builtin.Int64, %6 : $Builtin.Int1) : $(Builtin.Int64, Builtin.Int1)
%10 = tuple_extract %9 : $(Builtin.Int64, Builtin.Int1), 0
%11 = struct $Int (%10 : $Builtin.Int64)
%12 = builtin "cmp_eq_Int64"(%10 : $Builtin.Int64, %1 : $Builtin.Int64) : $Builtin.Int1
strong_retain %box : $<τ_0_0> { var τ_0_0 } <Builtin.Int64>
apply %funcref (%box) : $@convention(thin) (@guaranteed <τ_0_0> { var τ_0_0 } <Builtin.Int64>) -> ()
strong_release %box : $<τ_0_0> { var τ_0_0 } <Builtin.Int64>
cond_br %12, bb3, bb2

bb2:
br bb1(%10 : $Builtin.Int64)

bb3:
strong_release %box : $<τ_0_0> { var τ_0_0 } <Builtin.Int64>
%17 = tuple ()
return %17 : $()
}

// CHECK-LABEL: sil @silargument_retain_iterated : $@convention(thin) (@owned <τ_0_0> { var τ_0_0 } <Builtin.Int32>) -> ()
// CHECK: bb0
// CHECK-NEXT: function_ref user
Expand Down