Skip to content

[semantic-arc-opts] When performing load [copy] -> load_borrow on classes, do not ignore forwarding uses. #28989

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.

Sign up for GitHub

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
Jan 3, 2020
Merged

[semantic-arc-opts] When performing load [copy] -> load_borrow on classes, do not ignore forwarding uses. #28989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 34 additions & 2 deletions lib/SILOptimizer/Mandatory/SemanticARCOpts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,25 @@ STATISTIC(NumEliminatedInsts, "number of removed instructions");
STATISTIC(NumLoadCopyConvertedToLoadBorrow,
"number of load_copy converted to load_borrow");

//===----------------------------------------------------------------------===//
// Utility
//===----------------------------------------------------------------------===//

static bool isConsumingOwnedUse(Operand *use) {
assert(use->get().getOwnershipKind() == ValueOwnershipKind::Owned);

if (use->isTypeDependent())
return false;

// We know that a copy_value produces an @owned value. Look through all of
// our uses and classify them as either invalidating or not
// invalidating. Make sure that all of the invalidating ones are
// destroy_value since otherwise the live_range is not complete.
auto map = use->getOwnershipKindMap();
auto constraint = map.getLifetimeConstraint(ValueOwnershipKind::Owned);
return constraint == UseLifetimeConstraint::MustBeInvalidated;
}

//===----------------------------------------------------------------------===//
// Live Range Modeling
//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -775,8 +794,21 @@ class StorageGuaranteesLoadVisitor
std::back_inserter(baseEndBorrows));

SmallVector<SILInstruction *, 4> valueDestroys;
llvm::copy(Load->getUsersOfType<DestroyValueInst>(),
std::back_inserter(valueDestroys));
for (auto *use : Load->getUses()) {
// If this load is not consuming, skip it.
if (!isConsumingOwnedUse(use)) {
continue;
}

// Otherwise, if this isn't a destroy_value, we have a consuming use we
// don't understand. Return conservatively that this memory location may
// not be guaranteed.
auto *user = use->getUser();
if (!isa<DestroyValueInst>(user)) {
return answer(true);
}
valueDestroys.emplace_back(user);
}

SmallPtrSet<SILBasicBlock *, 4> visitedBlocks;
LinearLifetimeChecker checker(visitedBlocks, ARCOpt.getDeadEndBlocks());
Expand Down
74 changes: 61 additions & 13 deletions test/SILOptimizer/semantic-arc-opts.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ import Builtin
//////////////////

enum MyNever {}
enum FakeOptional<T> {
case none
case some(T)
}

sil @guaranteed_user : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
sil @owned_user : $@convention(thin) (@owned Builtin.NativeObject) -> ()
Expand All @@ -24,6 +28,7 @@ sil @get_nativeobject_pair : $@convention(thin) () -> @owned NativeObjectPair

class Klass {}
sil @guaranteed_klass_user : $@convention(thin) (@guaranteed Klass) -> ()
sil @guaranteed_fakeoptional_klass_user : $@convention(thin) (@guaranteed FakeOptional<Klass>) -> ()

struct MyInt {
var value: Builtin.Int32
Expand All @@ -40,11 +45,6 @@ struct StructMemberTest {
var t : (Builtin.Int32, AnotherStruct)
}

enum FakeOptional<T> {
case none
case some(T)
}

class ClassLet {
@_hasStorage let aLet: Klass
@_hasStorage var aVar: Klass
Expand Down Expand Up @@ -589,6 +589,52 @@ bb0(%x : @owned $ClassLet):
return undef : $()
}

// We do not support this today, but we will once forwarding is ignored when
// checking if the load [copy] is a dead live range.
//
// CHECK-LABEL: sil [ossa] @dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase :
// CHECK: load [copy]
// CHECK: } // end sil function 'dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase'
sil [ossa] @dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase : $@convention(thin) (@owned ClassLet) -> () {
bb0(%x : @owned $ClassLet):
%f = function_ref @black_hole : $@convention(thin) (@guaranteed Klass) -> ()

%a = begin_borrow %x : $ClassLet
%p = ref_element_addr %a : $ClassLet, #ClassLet.aLetTuple
%v = load [copy] %p : $*(Klass, Klass)
(%v1, %v2) = destructure_tuple %v : $(Klass, Klass)
apply %f(%v1) : $@convention(thin) (@guaranteed Klass) -> ()
apply %f(%v2) : $@convention(thin) (@guaranteed Klass) -> ()
destroy_value %v1 : $Klass
destroy_value %v2 : $Klass
end_borrow %a : $ClassLet
destroy_value %x : $ClassLet

return undef : $()
}

// We do not support this today, but we will once forwarding is ignored when
// checking if the load [copy] is a dead live range.
//
// CHECK-LABEL: sil [ossa] @dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase_2 :
// CHECK: load [copy]
// CHECK: } // end sil function 'dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase_2'
sil [ossa] @dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase_2 : $@convention(thin) (@owned ClassLet) -> () {
bb0(%x : @owned $ClassLet):
%f = function_ref @guaranteed_user : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()

%a = begin_borrow %x : $ClassLet
%p = ref_element_addr %a : $ClassLet, #ClassLet.aLet
%v = load [copy] %p : $*Klass
%v_cast = unchecked_ref_cast %v : $Klass to $Builtin.NativeObject
apply %f(%v_cast) : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
destroy_value %v_cast : $Builtin.NativeObject
end_borrow %a : $ClassLet
destroy_value %x : $ClassLet

return undef : $()
}

// CHECK-LABEL: sil [ossa] @dont_copy_let_properties_with_multi_borrowed_base_that_dominates
// CHECK: [[OUTER:%.*]] = begin_borrow
// CHECK-NEXT: ref_element_addr
Expand Down Expand Up @@ -662,7 +708,7 @@ bb0(%x : @owned $ClassLet):
return undef : $()
}

// CHECK-LABEL: sil [ossa] @do_or_dont_copy_let_properties_with_multi_borrowed_base_when_it_dominates
// CHECK-LABEL: sil [ossa] @do_or_dont_copy_let_properties_with_multi_borrowed_base_when_it_dominates_2 :
// CHECK: [[OUTER:%.*]] = begin_borrow
// CHECK-NEXT: ref_element_addr
// CHECK-NEXT: [[INNER:%.*]] = load_borrow
Expand All @@ -672,12 +718,15 @@ bb0(%x : @owned $ClassLet):
// CHECK-NEXT: begin_borrow
// CHECK-NEXT: ref_element_addr
// CHECK-NEXT: load [copy]
// CHECK-NEXT: apply
// CHECK-NEXT: end_borrow
// CHECK-NEXT: destroy_value
// CHECK-NEXT: // function_ref
// CHECK-NEXT: function_ref
// CHECK-NEXT: enum
// CHECK-NEXT: apply
// CHECK-NEXT: destroy_value
sil [ossa] @do_or_dont_copy_let_properties_with_multi_borrowed_base_when_it_dominates : $@convention(thin) (@owned ClassLet) -> () {
// CHECK: } // end sil function 'do_or_dont_copy_let_properties_with_multi_borrowed_base_when_it_dominates_2'
sil [ossa] @do_or_dont_copy_let_properties_with_multi_borrowed_base_when_it_dominates_2 : $@convention(thin) (@owned ClassLet) -> () {
bb0(%x : @owned $ClassLet):
%f = function_ref @black_hole : $@convention(thin) (@guaranteed Klass) -> ()

Expand All @@ -693,18 +742,17 @@ bb0(%x : @owned $ClassLet):
%b = begin_borrow %x : $ClassLet
%q = ref_element_addr %b : $ClassLet, #ClassLet.aLet
%w = load [copy] %q : $*Klass
%d = begin_borrow %w : $Klass
apply %f(%d) : $@convention(thin) (@guaranteed Klass) -> ()

// End the lifetime of the base object first...
end_borrow %b : $ClassLet
destroy_value %x : $ClassLet

// ...then end the lifetime of the copy.
apply %f(%d) : $@convention(thin) (@guaranteed Klass) -> ()
%f2 = function_ref @guaranteed_fakeoptional_klass_user : $@convention(thin) (@guaranteed FakeOptional<Klass>) -> ()
%w2 = enum $FakeOptional<Klass>, #FakeOptional.some!enumelt.1, %w : $Klass
apply %f2(%w2) : $@convention(thin) (@guaranteed FakeOptional<Klass>) -> ()

end_borrow %d : $Klass
destroy_value %w : $Klass
destroy_value %w2 : $FakeOptional<Klass>

return undef : $()
}
Expand Down