Skip to content

Commit b90e413

Browse files
gottesmmgottesmm
authored
Merge pull request #25320 from gottesmm/pr-393856a2bc74013494a4598e46d156795de076f2
[clf] Simplify the double diamond lifetime extension for escaping swi…
2 parents bab64fb + cf0405c commit b90e413

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;

‎test/SILOptimizer/closure-lifetime-fixup.sil

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)