Skip to content

Commit 0e05b51

Browse files
gottesmmgottesmm
committed
When converting load [copy] -> load_borrow, do not insert end_borrow if the block insert point is a dead end block.
This is important to do since otherwise, we may be implicitly reducing the lifetime of a value which we can not do yet since we do not require all interior pointer instructions to be guarded by borrows (yet). Once that constraint is in place, we will not have this problem. Consider a situation where one has a @owned switch_enum on an indirect box case which is post-dominated by an unreachable that we want to convert to @guaranteed: enum MyEnum { indirect case FirstCase(Int) ... } bb0(%in_guaranteed_addr : $*MyEnum): ... %0 = load [copy] %in_guaranteed_addr : $*MyEnum switch_enum %0 : $MyEnum, case #MyEnum.FirstCase: bb1, ... bb1(%1 : @owned ${ var Int }): %2 = project_box %1 : ${ var Int }, 0 %3 = load [trivial] %2 : $*Int apply %log(%3) : $@convention(thin) (Int) -> () unreachable In this case, we will not have a destroy_value on the box, but we may have a project_box on the box. This is ok since we are going to leak the value. But since we are using all consuming uses to determine the lifetime, we will want to insert an end_borrow at the head of the switch_enum dest block like follows: bb0(%in_guaranteed_addr : $*MyEnum): ... %0 = load_borrow %in_guaranteed_addr : $*MyEnum switch_enum %0 : $MyEnum, case #MyEnum.FirstCase: bb1, ... bb1(%1 : @guaranteed ${ var Int }): end_borrow %1 : ${ var Int } %2 = project_box %1 : ${ var Int }, 0 %3 = load [trivial] %2 : $*Int apply %log(%3) : $@convention(thin) (Int) -> () unreachable which would violate ownership invariants. Instead, we need to realize that %1 is dominated by a dead end block so we may not have a destroy_value upon it meaning we should just not insert the end_borrow here. If we have a destroy_value upon it (since we did not get rid of a destroy_value), then we will still get rid of the destroy_value if we are going to optimize this, so we are still correct. rdar://68096662
1 parent 64d4465 commit 0e05b51

File tree

2 files changed

+136
-0
lines changed

2 files changed

+136
-0
lines changed

‎lib/SILOptimizer/SemanticARC/OwnershipLiveRange.cpp

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
#include "OwnershipLiveRange.h"
1414
#include "OwnershipPhiOperand.h"
1515

16+
#include "swift/SIL/BasicBlockUtils.h"
17+
1618
using namespace swift;
1719
using namespace swift::semanticarc;
1820

@@ -162,6 +164,61 @@ void OwnershipLiveRange::insertEndBorrowsAtDestroys(
162164
auto loc = RegularLocation::getAutoGeneratedLocation();
163165
while (!scratch.empty()) {
164166
auto *insertPoint = scratch.pop_back_val();
167+
168+
// Do not insert end_borrow if the block insert point is a dead end block.
169+
//
170+
// DISCUSSION: This is important to do since otherwise, we may be implicitly
171+
// reducing the lifetime of a value which we can not do yet since we do not
172+
// require all interior pointer instructions to be guarded by borrows
173+
// (yet). Once that constraint is in place, we will not have this problem.
174+
//
175+
// Consider a situation where one has a @owned switch_enum on an
176+
// indirect box case which is post-dominated by an unreachable that we want
177+
// to convert to @guaranteed:
178+
//
179+
// enum MyEnum {
180+
// indirect case FirstCase(Int)
181+
// ...
182+
// }
183+
//
184+
// bb0(%in_guaranteed_addr : $*MyEnum):
185+
// ...
186+
// %0 = load [copy] %in_guaranteed_addr : $*MyEnum
187+
// switch_enum %0 : $MyEnum, case #MyEnum.FirstCase: bb1, ...
188+
//
189+
// bb1(%1 : @owned ${ var Int }):
190+
// %2 = project_box %1 : ${ var Int }, 0
191+
// %3 = load [trivial] %2 : $*Int
192+
// apply %log(%3) : $@convention(thin) (Int) -> ()
193+
// unreachable
194+
//
195+
// In this case, we will not have a destroy_value on the box, but we may
196+
// have a project_box on the box. This is ok since we are going to leak the
197+
// value. But since we are using all consuming uses to determine the
198+
// lifetime, we will want to insert an end_borrow at the head of the
199+
// switch_enum dest block like follows:
200+
//
201+
// bb0(%in_guaranteed_addr : $*MyEnum):
202+
// ...
203+
// %0 = load_borrow %in_guaranteed_addr : $*MyEnum
204+
// switch_enum %0 : $MyEnum, case #MyEnum.FirstCase: bb1, ...
205+
//
206+
// bb1(%1 : @guaranteed ${ var Int }):
207+
// end_borrow %1 : ${ var Int }
208+
// %2 = project_box %1 : ${ var Int }, 0
209+
// %3 = load [trivial] %2 : $*Int
210+
// apply %log(%3) : $@convention(thin) (Int) -> ()
211+
// unreachable
212+
//
213+
// which would violate ownership invariants. Instead, we need to realize
214+
// that %1 is dominated by a dead end block so we may not have a
215+
// destroy_value upon it meaning we should just not insert the end_borrow
216+
// here. If we have a destroy_value upon it (since we did not get rid of a
217+
// destroy_value), then we will still get rid of the destroy_value if we are
218+
// going to optimize this, so we are still correct.
219+
if (deadEndBlocks.isDeadEnd(insertPoint->getParent()))
220+
continue;
221+
165222
SILBuilderWithScope builder(insertPoint);
166223
builder.createEndBorrow(loc, newGuaranteedValue);
167224
}

‎test/SILOptimizer/semantic-arc-opts.sil

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,16 @@ class SubclassLet: ClassLet {}
7979
sil_global [let] @a_let_global : $Klass
8080
sil_global @a_var_global : $Klass
8181

82+
enum EnumWithIndirectCase {
83+
case first
84+
indirect case second(Builtin.NativeObject)
85+
}
86+
87+
struct StructWithEnumWithIndirectCaseField {
88+
var i: Builtin.Int23
89+
var field : EnumWithIndirectCase
90+
}
91+
8292
///////////
8393
// Tests //
8494
///////////
@@ -2727,3 +2737,72 @@ bbEnd:
27272737
return %9999 : $()
27282738
}
27292739

2740+
// CHECK-LABEL: sil [ossa] @enum_with_indirect_case_projectbox_copyvalue_deadend : $@convention(thin) (@guaranteed StructWithEnumWithIndirectCaseField) -> () {
2741+
// CHECK-NOT: copy_value
2742+
// CHECK: } // end sil function 'enum_with_indirect_case_projectbox_copyvalue_deadend'
2743+
sil [ossa] @enum_with_indirect_case_projectbox_copyvalue_deadend : $@convention(thin) (@guaranteed StructWithEnumWithIndirectCaseField) -> () {
2744+
bb0(%0 : @guaranteed $StructWithEnumWithIndirectCaseField):
2745+
%1 = struct_extract %0 : $StructWithEnumWithIndirectCaseField, #StructWithEnumWithIndirectCaseField.field
2746+
%1a = copy_value %1 : $EnumWithIndirectCase
2747+
switch_enum %1a : $EnumWithIndirectCase, case #EnumWithIndirectCase.first!enumelt: bb1, case #EnumWithIndirectCase.second!enumelt: bb2
2748+
2749+
bb1:
2750+
%9999 = tuple()
2751+
return %9999 : $()
2752+
2753+
// NOTE: Eventually this will need to be changed when project_box has to be
2754+
// guarded by begin_borrow.
2755+
bb2(%2 : @owned ${ var Builtin.NativeObject }):
2756+
%3 = project_box %2 : ${ var Builtin.NativeObject }, 0
2757+
%4 = load [copy] %3 : $*Builtin.NativeObject
2758+
%user = function_ref @guaranteed_user : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
2759+
apply %user(%4) : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
2760+
unreachable
2761+
}
2762+
2763+
// CHECK-LABEL: sil [ossa] @enum_with_indirect_case_projectbox_loadcopy_to_loadborrow_deadend : $@convention(thin) (@in_guaranteed EnumWithIndirectCase) -> () {
2764+
// CHECK: bb0
2765+
// CHECK-NEXT: load_borrow
2766+
// CHECK: } // end sil function 'enum_with_indirect_case_projectbox_loadcopy_to_loadborrow_deadend'
2767+
sil [ossa] @enum_with_indirect_case_projectbox_loadcopy_to_loadborrow_deadend : $@convention(thin) (@in_guaranteed EnumWithIndirectCase) -> () {
2768+
bb0(%0 : $*EnumWithIndirectCase):
2769+
%1 = load [copy] %0 : $*EnumWithIndirectCase
2770+
switch_enum %1 : $EnumWithIndirectCase, case #EnumWithIndirectCase.first!enumelt: bb1, case #EnumWithIndirectCase.second!enumelt: bb2
2771+
2772+
bb1:
2773+
%9999 = tuple()
2774+
return %9999 : $()
2775+
2776+
// NOTE: Eventually this will need to be changed when project_box has to be
2777+
// guarded by begin_borrow.
2778+
bb2(%2 : @owned ${ var Builtin.NativeObject }):
2779+
%3 = project_box %2 : ${ var Builtin.NativeObject }, 0
2780+
%4 = load [copy] %3 : $*Builtin.NativeObject
2781+
%user = function_ref @guaranteed_user : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
2782+
apply %user(%4) : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
2783+
unreachable
2784+
}
2785+
2786+
// CHECK-LABEL: sil [ossa] @enum_with_indirect_case_projectbox_loadcopy_to_loadborrow_deadend_2 : $@convention(thin) (@in_guaranteed EnumWithIndirectCase) -> () {
2787+
// CHECK: bb0
2788+
// CHECK-NEXT: load_borrow
2789+
// CHECK: } // end sil function 'enum_with_indirect_case_projectbox_loadcopy_to_loadborrow_deadend_2'
2790+
sil [ossa] @enum_with_indirect_case_projectbox_loadcopy_to_loadborrow_deadend_2 : $@convention(thin) (@in_guaranteed EnumWithIndirectCase) -> () {
2791+
bb0(%0 : $*EnumWithIndirectCase):
2792+
%1 = load [copy] %0 : $*EnumWithIndirectCase
2793+
switch_enum %1 : $EnumWithIndirectCase, case #EnumWithIndirectCase.first!enumelt: bb1, case #EnumWithIndirectCase.second!enumelt: bb2
2794+
2795+
bb1:
2796+
%9999 = tuple()
2797+
return %9999 : $()
2798+
2799+
// NOTE: Eventually this will need to be changed when project_box has to be
2800+
// guarded by begin_borrow.
2801+
bb2(%2 : @owned ${ var Builtin.NativeObject }):
2802+
%3 = project_box %2 : ${ var Builtin.NativeObject }, 0
2803+
%4 = load [copy] %3 : $*Builtin.NativeObject
2804+
%user = function_ref @guaranteed_user : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
2805+
apply %user(%4) : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
2806+
destroy_value %2 : ${ var Builtin.NativeObject }
2807+
unreachable
2808+
}

0 commit comments

Comments
 (0)