Skip to content

Commit cf0405c

Browse files
committed
[clf] Simplify the double diamond lifetime extension for escaping swift funcs -> noescape blocks.
Specifically, when we optimize conversions such as: Optional<@escaping () -> ()> -> Optional<@NoEscape () -> ()> -> Optional<@NoEscape @convention(block) () -> ()> previously we were lifetime extending over the @NoEscape lifetime barrier by making a copy and then putting a mark_dependence from the copy onto the original value. This was just a quick way to tell the ownership verifier that the copy was tied to the other value and thus should not be eliminated. The correctness of the actual lifetime extension comes from the optimizer being conservative around rr insts. This commit instead changes our optimization to borrow the copied optional value, extract the payload, and use that instead.
1 parent 07e6940 commit cf0405c

File tree

4 files changed

+209
-30
lines changed

4 files changed

+209
-30
lines changed

include/swift/SIL/SILBuilder.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1279,6 +1279,13 @@ class SILBuilder {
12791279
return createUncheckedEnumData(Loc, Operand, Element, EltType);
12801280
}
12811281

1282+
/// Return unchecked_enum_data %Operand, #Optional<T>.some.
1283+
SILValue emitExtractOptionalPayloadOperation(SILLocation Loc,
1284+
SILValue Operand) {
1285+
auto *Decl = F->getASTContext().getOptionalSomeDecl();
1286+
return createUncheckedEnumData(Loc, Operand, Decl);
1287+
}
1288+
12821289
UncheckedTakeEnumDataAddrInst *
12831290
createUncheckedTakeEnumDataAddr(SILLocation Loc, SILValue Operand,
12841291
EnumElementDecl *Element, SILType Ty) {

lib/SILOptimizer/Mandatory/ClosureLifetimeFixup.cpp

Lines changed: 83 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,17 @@ llvm::cl::opt<bool> DisableConvertEscapeToNoEscapeSwitchEnumPeephole(
3535

3636
using namespace swift;
3737

38+
/// Given an optional diamond, return the bottom of the diamond.
39+
///
40+
/// That is given that sei is in bb0,
41+
///
42+
/// /---> bb1 ---\
43+
/// / \
44+
/// bb0 ---> bb3
45+
/// \ /
46+
/// \---> bb2 ---/
47+
///
48+
/// this routine will return bb3.
3849
static SILBasicBlock *getOptionalDiamondSuccessor(SwitchEnumInst *sei) {
3950
auto numSuccs = sei->getNumSuccessors();
4051
if (numSuccs != 2)
@@ -434,25 +445,53 @@ static bool tryExtendLifetimeToLastUse(
434445
return false;
435446
}
436447

437-
/// Ensure the lifetime of the closure accross an
448+
/// Ensure the lifetime of the closure across a two step optional conversion
449+
/// from:
450+
///
451+
/// optional<@escaping () -> ()>
452+
///
453+
/// to:
454+
///
455+
/// optional<@noescape () -> ()>
456+
///
457+
/// to:
438458
///
439-
/// optional<@escaping () -> ()> to
440459
/// optional<@noescape @convention(block) () -> ()>
441460
///
442-
/// conversion and its use.
461+
/// and all uses of the block. The pattern that we are looking for is:
443462
///
444-
/// The pattern this is looking for
445-
/// switch_enum %closure
446-
/// / \
447-
/// convert_escape_to_noescape nil
448-
/// switch_enum
463+
/// switch_enum %optional_closure (1)
449464
/// / \
450-
/// convertToBlock nil
465+
/// %trivial_closure = CVT %closure nil (2)
466+
/// \ /
467+
/// switch_enum %optional_trivial_closure (3)
468+
/// / \
469+
/// %b = convertToBlock %trivial_closure nil (4)
451470
/// \ /
452-
/// (%convertOptionalBlock :)
453-
/// We will insert a copy_value of the original %closure before the two
454-
/// diamonds. And a destroy of %closure at the last destroy of
455-
/// %convertOptionalBlock.
471+
/// ... uses of %optional_block ...
472+
/// destroy_value %optional_block
473+
///
474+
/// where CVT is convert_escape_to_no_escape [not_guaranteed]. We assume that
475+
/// the %optional_block is going through a conversion sequence in SILGen meaning
476+
/// that we should only have a single destroy of the optional block.
477+
///
478+
/// NOTE: There is a *lifetime gap* during the usage of the trivial_closure!
479+
/// This means we must be careful when lifetime extending. We can only assume
480+
/// that the underlying closure is alive immediately at the CVT. So to perform
481+
/// our lifetime extend, we do the following:
482+
///
483+
/// 1. We copy and borrow optional_closure, right before the switch_enum in
484+
/// (1).
485+
///
486+
/// 2. We rewrite the convert_escape_to_no_escape guaranteed to use the copy
487+
/// instead.
488+
///
489+
/// 3. To make sure that even after ossa is complete, we do not move any
490+
/// destroys above the convert_escape_to_no_escape by putting a mark_dependence
491+
/// on %closure
492+
///
493+
/// 4. We insert an end_borrow, destroy for the copy at the destroy of the
494+
/// optional block.
456495
static bool trySwitchEnumPeephole(ConvertEscapeToNoEscapeInst *cvt) {
457496
auto *blockArg = dyn_cast<SILArgument>(cvt->getOperand());
458497
if (!blockArg)
@@ -478,7 +517,8 @@ static bool trySwitchEnumPeephole(ConvertEscapeToNoEscapeInst *cvt) {
478517
if (diamondSucc2->getNumArguments() != 1)
479518
return false;
480519

481-
// Look for the last and only destroy.
520+
// Look for the last and only destroy of the diamond succ 2's argument. This
521+
// is going to be the place where we destroy the lifetime extending copy.
482522
SILInstruction *onlyDestroy = [&]() -> SILInstruction * {
483523
SILInstruction *lastDestroy = nullptr;
484524
for (auto *use : diamondSucc2->getArgument(0)->getUses()) {
@@ -497,15 +537,27 @@ static bool trySwitchEnumPeephole(ConvertEscapeToNoEscapeInst *cvt) {
497537

498538
// Extend the lifetime.
499539
auto loc = RegularLocation::getAutoGeneratedLocation();
500-
auto *copy = SILBuilderWithScope(switchEnum1)
501-
.createCopyValue(loc, switchEnum1->getOperand());
540+
SILValue copy, borrow;
541+
std::tie(copy, borrow) = ([&]() -> std::pair<SILValue, SILValue> {
542+
SILBuilderWithScope builder(switchEnum1);
543+
auto copy = builder.emitCopyValueOperation(loc, switchEnum1->getOperand());
544+
auto borrow = builder.emitBeginBorrowOperation(loc, copy);
545+
return {copy, borrow};
546+
})(); // end std::tie(copy, borrow).
547+
548+
{
549+
SILBuilderWithScope builder(cvt);
550+
auto value = builder.emitExtractOptionalPayloadOperation(loc, borrow);
551+
cvt->setOperand(value);
552+
cvt->setLifetimeGuaranteed();
553+
}
502554

503-
cvt->setLifetimeGuaranteed();
504-
auto *mdi = SILBuilderWithScope(cvt).createMarkDependence(
505-
loc, cvt->getOperand(), copy);
506-
cvt->setOperand(mdi);
555+
{
556+
SILBuilderWithScope builder(onlyDestroy);
557+
builder.emitEndBorrowOperation(loc, borrow);
558+
builder.emitDestroyValueOperation(loc, copy);
559+
}
507560

508-
SILBuilderWithScope(onlyDestroy).createDestroyValue(loc, copy);
509561
return true;
510562
}
511563

@@ -757,7 +809,9 @@ static bool fixupClosureLifetimes(SILFunction &fn, bool &checkStackNesting,
757809

758810
// Handle, copy_block_without_escaping instructions.
759811
if (auto *cb = dyn_cast<CopyBlockWithoutEscapingInst>(inst)) {
760-
changed |= fixupCopyBlockWithoutEscaping(cb, modifiedCFG);
812+
if (fixupCopyBlockWithoutEscaping(cb, modifiedCFG)) {
813+
changed = true;
814+
}
761815
continue;
762816
}
763817

@@ -768,22 +822,23 @@ static bool fixupClosureLifetimes(SILFunction &fn, bool &checkStackNesting,
768822
continue;
769823

770824
// First try to peephole a known pattern.
771-
if (!DisableConvertEscapeToNoEscapeSwitchEnumPeephole &&
772-
trySwitchEnumPeephole(cvt)) {
773-
changed |= true;
774-
continue;
825+
if (!DisableConvertEscapeToNoEscapeSwitchEnumPeephole) {
826+
if (trySwitchEnumPeephole(cvt)) {
827+
changed = true;
828+
continue;
829+
}
775830
}
776831

777832
if (tryExtendLifetimeToLastUse(cvt, memoizedQueries, i)) {
778-
changed |= true;
833+
changed = true;
779834
checkStackNesting = true;
780835
continue;
781836
}
782837

783838
// Otherwise, extend the lifetime of the operand to the end of the
784839
// function.
785840
extendLifetimeToEndOfFunction(fn, cvt);
786-
changed |= true;
841+
changed = true;
787842
}
788843
}
789844
return changed;
Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
// RUN: %target-sil-opt -closure-lifetime-fixup %s | %FileCheck %s
2+
3+
sil_stage raw
4+
5+
import Swift
6+
import Builtin
7+
import SwiftShims
8+
9+
class FakeNSString {}
10+
11+
sil @$sSSSgIgg_AAIegg_TR : $@convention(thin) (@guaranteed Optional<String>, @noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()) -> ()
12+
sil @noescapeBlock3 : $@convention(c) (Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, Optional<FakeNSString>) -> ()
13+
sil @$sSS10FoundationE19_bridgeToObjectiveCSo8FakeNSStringCyF : $@convention(method) (@guaranteed String) -> @owned FakeNSString
14+
sil @$sSS21_builtinStringLiteral17utf8CodeUnitCount7isASCIISSBp_BwBi1_tcfC : $@convention(method) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, @thin String.Type) -> @owned String
15+
sil @$sSSSgIegg_So8FakeNSStringCSgIyBy_TR : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed (@guaranteed Optional<String>) -> (), Optional<FakeNSString>) -> ()
16+
17+
// Just make sure that we perform the optimization and do not trigger the ownership verifier.
18+
//
19+
// CHECK-LABEL: sil [ossa] @test1 : $@convention(thin) (@guaranteed Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>, @guaranteed Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>) -> () {
20+
// CHECK-NOT: convert_escape_to_noescape [not_guaranteed]
21+
// CHECK: } // end sil function 'test1'
22+
sil [ossa] @test1 : $@convention(thin) (@guaranteed Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>, @guaranteed Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>) -> () {
23+
bb0(%0 : @guaranteed $Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>, %1 : @guaranteed $Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>):
24+
%2 = copy_value %0 : $Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>
25+
switch_enum %2 : $Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb12
26+
27+
bb1(%4 : @owned $@callee_guaranteed (@guaranteed Optional<String>) -> ()):
28+
%5 = convert_escape_to_noescape [not_guaranteed] %4 : $@callee_guaranteed (@guaranteed Optional<String>) -> () to $@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()
29+
%6 = enum $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>, #Optional.some!enumelt.1, %5 : $@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()
30+
destroy_value %4 : $@callee_guaranteed (@guaranteed Optional<String>) -> ()
31+
br bb2(%6 : $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>)
32+
33+
bb2(%9 : $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>):
34+
switch_enum %9 : $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>, case #Optional.some!enumelt.1: bb3, case #Optional.none!enumelt: bb4
35+
36+
bb3(%11 : $@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()):
37+
%12 = function_ref @$sSSSgIgg_AAIegg_TR : $@convention(thin) (@guaranteed Optional<String>, @noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()) -> ()
38+
%13 = partial_apply [callee_guaranteed] %12(%11) : $@convention(thin) (@guaranteed Optional<String>, @noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()) -> ()
39+
%14 = mark_dependence %13 : $@callee_guaranteed (@guaranteed Optional<String>) -> () on %11 : $@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()
40+
%15 = copy_value %14 : $@callee_guaranteed (@guaranteed Optional<String>) -> ()
41+
%16 = alloc_stack $@block_storage @callee_guaranteed (@guaranteed Optional<String>) -> ()
42+
%17 = project_block_storage %16 : $*@block_storage @callee_guaranteed (@guaranteed Optional<String>) -> ()
43+
store %15 to [init] %17 : $*@callee_guaranteed (@guaranteed Optional<String>) -> ()
44+
%19 = function_ref @$sSSSgIegg_So8FakeNSStringCSgIyBy_TR : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed (@guaranteed Optional<String>) -> (), Optional<FakeNSString>) -> ()
45+
%20 = init_block_storage_header %16 : $*@block_storage @callee_guaranteed (@guaranteed Optional<String>) -> (), invoke %19 : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed (@guaranteed Optional<String>) -> (), Optional<FakeNSString>) -> (), type $@convention(block) @noescape (Optional<FakeNSString>) -> ()
46+
%21 = copy_block_without_escaping %20 : $@convention(block) @noescape (Optional<FakeNSString>) -> () withoutEscaping %14 : $@callee_guaranteed (@guaranteed Optional<String>) -> ()
47+
%22 = enum $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, #Optional.some!enumelt.1, %21 : $@convention(block) @noescape (Optional<FakeNSString>) -> ()
48+
destroy_addr %17 : $*@callee_guaranteed (@guaranteed Optional<String>) -> ()
49+
dealloc_stack %16 : $*@block_storage @callee_guaranteed (@guaranteed Optional<String>) -> ()
50+
br bb5(%22 : $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>)
51+
52+
bb4:
53+
%26 = enum $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, #Optional.none!enumelt
54+
br bb5(%26 : $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>)
55+
56+
bb5(%28 : @owned $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>):
57+
%29 = copy_value %1 : $Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>
58+
switch_enum %29 : $Optional<@callee_guaranteed (@guaranteed Optional<String>) -> ()>, case #Optional.some!enumelt.1: bb6, case #Optional.none!enumelt: bb11
59+
60+
bb6(%31 : @owned $@callee_guaranteed (@guaranteed Optional<String>) -> ()):
61+
%32 = convert_escape_to_noescape [not_guaranteed] %31 : $@callee_guaranteed (@guaranteed Optional<String>) -> () to $@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()
62+
%33 = enum $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>, #Optional.some!enumelt.1, %32 : $@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()
63+
destroy_value %31 : $@callee_guaranteed (@guaranteed Optional<String>) -> ()
64+
br bb7(%33 : $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>)
65+
66+
bb7(%36 : $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>):
67+
switch_enum %36 : $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>, case #Optional.some!enumelt.1: bb8, case #Optional.none!enumelt: bb9
68+
69+
bb8(%38 : $@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()):
70+
%39 = function_ref @$sSSSgIgg_AAIegg_TR : $@convention(thin) (@guaranteed Optional<String>, @noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()) -> ()
71+
%40 = partial_apply [callee_guaranteed] %39(%38) : $@convention(thin) (@guaranteed Optional<String>, @noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()) -> ()
72+
%41 = mark_dependence %40 : $@callee_guaranteed (@guaranteed Optional<String>) -> () on %38 : $@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()
73+
%42 = copy_value %41 : $@callee_guaranteed (@guaranteed Optional<String>) -> ()
74+
%43 = alloc_stack $@block_storage @callee_guaranteed (@guaranteed Optional<String>) -> ()
75+
%44 = project_block_storage %43 : $*@block_storage @callee_guaranteed (@guaranteed Optional<String>) -> ()
76+
store %42 to [init] %44 : $*@callee_guaranteed (@guaranteed Optional<String>) -> ()
77+
%46 = function_ref @$sSSSgIegg_So8FakeNSStringCSgIyBy_TR : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed (@guaranteed Optional<String>) -> (), Optional<FakeNSString>) -> ()
78+
%47 = init_block_storage_header %43 : $*@block_storage @callee_guaranteed (@guaranteed Optional<String>) -> (), invoke %46 : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed (@guaranteed Optional<String>) -> (), Optional<FakeNSString>) -> (), type $@convention(block) @noescape (Optional<FakeNSString>) -> ()
79+
%48 = copy_block_without_escaping %47 : $@convention(block) @noescape (Optional<FakeNSString>) -> () withoutEscaping %41 : $@callee_guaranteed (@guaranteed Optional<String>) -> ()
80+
%49 = enum $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, #Optional.some!enumelt.1, %48 : $@convention(block) @noescape (Optional<FakeNSString>) -> ()
81+
destroy_addr %44 : $*@callee_guaranteed (@guaranteed Optional<String>) -> ()
82+
dealloc_stack %43 : $*@block_storage @callee_guaranteed (@guaranteed Optional<String>) -> ()
83+
br bb10(%49 : $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>)
84+
85+
bb9:
86+
%53 = enum $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, #Optional.none!enumelt
87+
br bb10(%53 : $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>)
88+
89+
bb10(%55 : @owned $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>):
90+
%56 = string_literal utf8 "Foobar"
91+
%57 = integer_literal $Builtin.Word, 6
92+
%58 = integer_literal $Builtin.Int1, -1
93+
%59 = metatype $@thin String.Type
94+
%60 = function_ref @$sSS21_builtinStringLiteral17utf8CodeUnitCount7isASCIISSBp_BwBi1_tcfC : $@convention(method) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, @thin String.Type) -> @owned String
95+
%61 = apply %60(%56, %57, %58, %59) : $@convention(method) (Builtin.RawPointer, Builtin.Word, Builtin.Int1, @thin String.Type) -> @owned String
96+
%62 = function_ref @$sSS10FoundationE19_bridgeToObjectiveCSo8FakeNSStringCyF : $@convention(method) (@guaranteed String) -> @owned FakeNSString
97+
%63 = begin_borrow %61 : $String
98+
%64 = apply %62(%63) : $@convention(method) (@guaranteed String) -> @owned FakeNSString
99+
end_borrow %63 : $String
100+
%66 = enum $Optional<FakeNSString>, #Optional.some!enumelt.1, %64 : $FakeNSString
101+
destroy_value %61 : $String
102+
%68 = function_ref @noescapeBlock3 : $@convention(c) (Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, Optional<FakeNSString>) -> ()
103+
%69 = apply %68(%28, %55, %66) : $@convention(c) (Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>, Optional<FakeNSString>) -> ()
104+
destroy_value %66 : $Optional<FakeNSString>
105+
destroy_value %55 : $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>
106+
destroy_value %28 : $Optional<@convention(block) @noescape (Optional<FakeNSString>) -> ()>
107+
%73 = tuple ()
108+
return %73 : $()
109+
110+
bb11:
111+
%75 = enum $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>, #Optional.none!enumelt
112+
br bb7(%75 : $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>)
113+
114+
bb12:
115+
%77 = enum $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>, #Optional.none!enumelt
116+
br bb2(%77 : $Optional<@noescape @callee_guaranteed (@guaranteed Optional<String>) -> ()>)
117+
}

test/SILOptimizer/definite-init-convert-to-escape.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,8 @@ public func returnOptionalEscape() -> (() ->())?
4646
// CHECK: switch_enum [[V1]] : $Optional<{{.*}}>, case #Optional.some!enumelt.1: [[SOME_BB:bb[0-9]+]], case #Optional.none!enumelt: [[NONE_BB:bb[0-9]+]]
4747
//
4848
// CHECK: [[SOME_BB]]([[V2:%.*]] : $@callee_guaranteed () -> ()):
49-
// CHECK: [[CVT_MARK_DEP:%.*]] = mark_dependence [[V2]] : $@callee_guaranteed () -> () on [[V1]]
50-
// CHECK: [[CVT:%.*]] = convert_escape_to_noescape [[CVT_MARK_DEP]]
49+
// CHECK: [[V1_UNWRAPPED:%.*]] = unchecked_enum_data [[V1]]
50+
// CHECK: [[CVT:%.*]] = convert_escape_to_noescape [[V1_UNWRAPPED]]
5151
// CHECK: [[SOME:%.*]] = enum $Optional<{{.*}}>, #Optional.some!enumelt.1, [[CVT]]
5252
// CHECK: strong_release [[V2]]
5353
// CHECK: br [[NEXT_BB:bb[0-9]+]]([[SOME]] :

0 commit comments

Comments
 (0)