Skip to content

Commit 3ccda34

Browse files
committed
Revert "[move-only] Ensure that we treat captured escaping closure arguments as such even if the closure doesn't actually escape"
This reverts commit 224674c. Originally, I made this change since we were going to follow the AST in a strict way in terms of what closures are considered escaping or not from a diagnostics perspective. Upon further investigation I found that we actually do something different for inout escaping semantics and by treating the AST as the one point of truth, we are being inconsistent with the rest of the compiler. As an example, the following code is considered by the compiler to not be an invalid escaping use of an inout implying that we do not consider the closure to be escaping: ``` func f(_ x: inout Int) { let g = { _ = x } } ``` in contrast, a var is always considered to be an escape: ``` func f(_ x: inout Int) { var g = { _ = x } } test2.swift:3:13: error: escaping closure captures 'inout' parameter 'x' var g = { ^ test2.swift:2:10: note: parameter 'x' is declared 'inout' func f(_ x: inout Int) { ^ test2.swift:4:11: note: captured here _ = x ^ ``` Of course, if we store the let into memory, we get the error one would expect: ``` var global: () -> () = {} func f(_ x: inout Int) { let g = { _ = x } global = g } test2.swift:4:11: error: escaping closure captures 'inout' parameter 'x' let g = { ^ test2.swift:3:10: note: parameter 'x' is declared 'inout' func f(_ x: inout Int) { ^ test2.swift:5:7: note: captured here _ = x ^ ``` By reverting to the old behavior where allocbox to stack ran early, noncopyable types now have the same sort of semantics: let closures that capture a noncopyable type that do not on the face of it escape are considered non-escaping, while if the closure is ever stored into memory (e.x.: store into a global, into a local var) or escapes, we get the appropriate escaping diagnostics. E.x.: ``` public struct E : ~Copyable {} public func borrowVal(_ e: borrowing E) {} public func consumeVal(_ e: consuming E) {} func f1() { var e = E() // Mutable borrowing use of e. We can consume e as long as we reinit at end // of function. We don't here, so we get an error. let c1: () -> () = { borrowVal(e) consumeVal(e) } // Mutable borrowing use of e. We can consume e as long as we reinit at end // of function. We do do that here, so no error. let c2: () -> () = { borrowVal(e) consumeVal(e) e = E() } } ```
1 parent 3865b5d commit 3ccda34

File tree

3 files changed

+14
-60
lines changed

3 files changed

+14
-60
lines changed

include/swift/SILOptimizer/PassManager/Passes.def

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,9 +112,6 @@ PASS(AccessMarkerElimination, "access-marker-elim",
112112
"Access Marker Elimination.")
113113
PASS(AddressLowering, "address-lowering",
114114
"SIL Address Lowering")
115-
PASS(EarlyAllocBoxToStack, "early-allocbox-to-stack",
116-
"Stack Promotion of Box Objects. Doesn't attempt to promote noncopyable "
117-
"types captured by escaping closures")
118115
PASS(AllocBoxToStack, "allocbox-to-stack",
119116
"Stack Promotion of Box Objects")
120117
IRGEN_PASS(AllocStackHoisting, "alloc-stack-hoisting",

lib/SILOptimizer/PassManager/PassPipeline.cpp

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ static void addMandatoryDiagnosticOptPipeline(SILPassPipelinePlan &P) {
125125
// This guarantees that stack-promotable boxes have [static] enforcement.
126126
P.addAccessEnforcementSelection();
127127

128-
P.addEarlyAllocBoxToStack();
128+
P.addAllocBoxToStack();
129129
P.addNoReturnFolding();
130130
addDefiniteInitialization(P);
131131

@@ -169,11 +169,6 @@ static void addMandatoryDiagnosticOptPipeline(SILPassPipelinePlan &P) {
169169
// End Ownership Optimizations
170170
//===---
171171

172-
// Now that we have finished checking noncopyable types, run later alloc box
173-
// to stack, so that we promote to the stack any heap boxes that are captured
174-
// by escaping closures where the closure does not actually escape.
175-
P.addAllocBoxToStack();
176-
177172
#ifndef NDEBUG
178173
// Add a verification pass to check our work when skipping
179174
// function bodies.

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

Lines changed: 13 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
#include "swift/AST/SemanticAttrs.h"
1717
#include "swift/Basic/BlotMapVector.h"
1818
#include "swift/Basic/GraphNodeWorklist.h"
19-
#include "swift/Basic/OptionSet.h"
2019
#include "swift/SIL/ApplySite.h"
2120
#include "swift/SIL/BasicBlockDatastructures.h"
2221
#include "swift/SIL/Dominance.h"
@@ -88,18 +87,6 @@ static bool useCaptured(Operand *UI) {
8887
return true;
8988
}
9089

91-
namespace {
92-
93-
enum class AllocBoxToStackFlags {
94-
InAppliedFunction = 0x1,
95-
CanPromoteNonCopyableCapturedByEscapingClosure = 0x2,
96-
HasMoveOnlyBox = 0x4,
97-
};
98-
99-
using AllocBoxToStackOptions = OptionSet<AllocBoxToStackFlags>;
100-
101-
} // namespace
102-
10390
//===----------------------------------------------------------------------===//
10491
// Liveness for alloc_box Promotion
10592
//===----------------------------------------------------------------------===//
@@ -314,7 +301,7 @@ static bool partialApplyEscapes(SILValue V, bool examineApply) {
314301
}
315302

316303
static SILInstruction *recursivelyFindBoxOperandsPromotableToAddress(
317-
SILValue Box, AllocBoxToStackOptions Options, SmallVectorImpl<Operand *> &,
304+
SILValue Box, bool inAppliedFunction, SmallVectorImpl<Operand *> &,
318305
SmallPtrSetImpl<SILFunction *> &, unsigned CurrentRecurDepth);
319306

320307
/// checkLocalApplyBody - Check the body of an apply's callee to see
@@ -324,8 +311,7 @@ static SILInstruction *recursivelyFindBoxOperandsPromotableToAddress(
324311
static bool checkLocalApplyBody(Operand *O,
325312
SmallVectorImpl<Operand *> &PromotedOperands,
326313
SmallPtrSetImpl<SILFunction *> &VisitedCallees,
327-
unsigned CurrentRecurDepth,
328-
AllocBoxToStackOptions Options) {
314+
unsigned CurrentRecurDepth) {
329315
SILFunction *F = ApplySite(O->getUser()).getReferencedFunctionOrNull();
330316
// If we cannot examine the function body, assume the worst.
331317
if (!F || F->empty())
@@ -337,11 +323,10 @@ static bool checkLocalApplyBody(Operand *O,
337323
if (!iter.second)
338324
return false;
339325

340-
Options |= {AllocBoxToStackFlags::InAppliedFunction};
341-
342326
auto calleeArg = F->getArgument(ApplySite(O->getUser()).getCalleeArgIndex(*O));
343327
auto res = !recursivelyFindBoxOperandsPromotableToAddress(
344-
calleeArg, Options, PromotedOperands, VisitedCallees,
328+
calleeArg,
329+
/* inAppliedFunction = */ true, PromotedOperands, VisitedCallees,
345330
CurrentRecurDepth + 1);
346331
return res;
347332
}
@@ -385,7 +370,7 @@ static bool isOptimizableApplySite(ApplySite Apply) {
385370
/// box is passed don't have any unexpected uses, `PromotedOperands` will be
386371
/// populated with the box arguments in DFS order.
387372
static SILInstruction *recursivelyFindBoxOperandsPromotableToAddress(
388-
SILValue Box, AllocBoxToStackOptions Options,
373+
SILValue Box, bool inAppliedFunction,
389374
SmallVectorImpl<Operand *> &PromotedOperands,
390375
SmallPtrSetImpl<SILFunction *> &VisitedCallees,
391376
unsigned CurrentRecurDepth = 0) {
@@ -409,8 +394,7 @@ static SILInstruction *recursivelyFindBoxOperandsPromotableToAddress(
409394
// Projections are fine as well.
410395
if (isa<StrongRetainInst>(User) || isa<StrongReleaseInst>(User) ||
411396
isa<ProjectBoxInst>(User) || isa<DestroyValueInst>(User) ||
412-
(!Options.contains(AllocBoxToStackFlags::InAppliedFunction) &&
413-
isa<DeallocBoxInst>(User)) ||
397+
(!inAppliedFunction && isa<DeallocBoxInst>(User)) ||
414398
isa<EndBorrowInst>(User))
415399
continue;
416400

@@ -429,17 +413,8 @@ static SILInstruction *recursivelyFindBoxOperandsPromotableToAddress(
429413
}
430414
switch (Apply.getKind()) {
431415
case ApplySiteKind::PartialApplyInst: {
432-
// If we have a noncopyable type in a box and we were passed in the
433-
// option that tells us that we should not promote any boxes that are
434-
// captured by an escaping closure (even if we can prove the closure
435-
// does not escape), treat the partial apply use as an escape.
436-
if (Box->getType().isBoxedNonCopyableType(Box->getFunction()) &&
437-
!Options.contains(
438-
AllocBoxToStackFlags::
439-
CanPromoteNonCopyableCapturedByEscapingClosure))
440-
break;
441416
if (checkLocalApplyBody(Op, LocalPromotedOperands, VisitedCallees,
442-
CurrentRecurDepth, Options) &&
417+
CurrentRecurDepth) &&
443418
!partialApplyEscapes(cast<PartialApplyInst>(User),
444419
/* examineApply = */ true)) {
445420
LocalPromotedOperands.push_back(Op);
@@ -452,7 +427,7 @@ static SILInstruction *recursivelyFindBoxOperandsPromotableToAddress(
452427
case ApplySiteKind::TryApplyInst:
453428
if (isOptimizableApplySite(Apply) &&
454429
checkLocalApplyBody(Op, LocalPromotedOperands, VisitedCallees,
455-
CurrentRecurDepth, Options)) {
430+
CurrentRecurDepth)) {
456431
LocalPromotedOperands.push_back(Op);
457432
continue;
458433
}
@@ -475,13 +450,13 @@ static InFlightDiagnostic diagnose(ASTContext &Context, SourceLoc loc,
475450

476451
/// canPromoteAllocBox - Can we promote this alloc_box to an alloc_stack?
477452
static bool canPromoteAllocBox(AllocBoxInst *ABI,
478-
SmallVectorImpl<Operand *> &PromotedOperands,
479-
AllocBoxToStackOptions Options) {
453+
SmallVectorImpl<Operand *> &PromotedOperands) {
480454
SmallPtrSet<SILFunction *, 8> VisitedCallees;
481455
// Scan all of the uses of the address of the box to see if any
482456
// disqualifies the box from being promoted to the stack.
483457
if (auto *User = recursivelyFindBoxOperandsPromotableToAddress(
484-
ABI, Options, PromotedOperands, VisitedCallees,
458+
ABI,
459+
/* inAppliedFunction = */ false, PromotedOperands, VisitedCallees,
485460
/* CurrentRecurDepth = */ 0)) {
486461
(void)User;
487462
// Otherwise, we have an unexpected use.
@@ -1246,8 +1221,6 @@ static unsigned rewritePromotedBoxes(AllocBoxToStackState &pass) {
12461221
}
12471222

12481223
namespace {
1249-
1250-
template <bool CanPromoteMoveOnlyCapturedByEscapingClosure>
12511224
class AllocBoxToStack : public SILFunctionTransform {
12521225
/// The entry point to the transformation.
12531226
void run() override {
@@ -1256,16 +1229,10 @@ class AllocBoxToStack : public SILFunctionTransform {
12561229
return;
12571230

12581231
AllocBoxToStackState pass(this);
1259-
1260-
AllocBoxToStackOptions Options;
1261-
if (CanPromoteMoveOnlyCapturedByEscapingClosure)
1262-
Options |=
1263-
AllocBoxToStackFlags::CanPromoteNonCopyableCapturedByEscapingClosure;
1264-
12651232
for (auto &BB : *getFunction()) {
12661233
for (auto &I : BB)
12671234
if (auto *ABI = dyn_cast<AllocBoxInst>(&I))
1268-
if (canPromoteAllocBox(ABI, pass.PromotedOperands, Options))
1235+
if (canPromoteAllocBox(ABI, pass.PromotedOperands))
12691236
pass.Promotable.push_back(ABI);
12701237
}
12711238

@@ -1284,13 +1251,8 @@ class AllocBoxToStack : public SILFunctionTransform {
12841251
}
12851252
}
12861253
};
1287-
12881254
} // end anonymous namespace
12891255

12901256
SILTransform *swift::createAllocBoxToStack() {
1291-
return new AllocBoxToStack<true>();
1292-
}
1293-
1294-
SILTransform *swift::createEarlyAllocBoxToStack() {
1295-
return new AllocBoxToStack<false>();
1257+
return new AllocBoxToStack();
12961258
}

0 commit comments

Comments
 (0)