Skip to content

Commit 9f0375e

Browse files
gottesmmgottesmm
committed
[silgenpattern] Only emit a shared case if we have a fallthrough/multiple cast items.
There was a bug here where we were emitting a shared case if we had arguments. That is why we needed the hacked in deletion of that block when we emitted shared blocks. I was able to replace that with an assert to make sure that we do not regress. This additionally improved our code generation by eliminating a potential extra copy when we had such a case. That is where the test updates come from.
1 parent 8c95abb commit 9f0375e

File tree

4 files changed

+57
-46
lines changed

4 files changed

+57
-46
lines changed

‎lib/SILGen/SILGenPattern.cpp‎

Lines changed: 34 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -2318,8 +2318,9 @@ void PatternMatchEmission::initSharedCaseBlockDest(CaseStmt *caseBlock,
23182318

23192319
auto *block = SGF.createBasicBlock();
23202320
result.first->second.first = block;
2321-
2322-
// Add args for any pattern variables
2321+
2322+
// If we do not have any bound decls, we do not need to setup any phi
2323+
// arguments for the shared case block. Just bail early.
23232324
if (!caseBlock->hasBoundDecls()) {
23242325
return;
23252326
}
@@ -2353,6 +2354,7 @@ void PatternMatchEmission::emitAddressOnlyAllocations() {
23532354
for (auto &entry : SharedCases) {
23542355
CaseStmt *caseBlock = entry.first;
23552356

2357+
// If we do not have any bound decls, we can bail early.
23562358
if (!caseBlock->hasBoundDecls()) {
23572359
continue;
23582360
}
@@ -2426,11 +2428,9 @@ void PatternMatchEmission::emitSharedCaseBlocks() {
24262428
SGF.B.setInsertionPoint(predBB);
24272429

24282430
} else {
2429-
// FIXME: Figure out why this is necessary.
2430-
if (caseBB->pred_empty()) {
2431-
SGF.eraseBasicBlock(caseBB);
2432-
continue;
2433-
}
2431+
// If we did not need a shared case block, we shouldn't have emitted one.
2432+
assert(!caseBB->pred_empty() &&
2433+
"Shared case block without predecessors?!");
24342434

24352435
// Otherwise, move the block to after the first predecessor.
24362436
auto predBB = *caseBB->pred_begin();
@@ -2646,32 +2646,36 @@ static void switchCaseStmtSuccessCallback(SILGenFunction &SGF,
26462646
auto caseBlock = row.getClientData<CaseStmt>();
26472647
SGF.emitProfilerIncrement(caseBlock);
26482648

2649-
// Certain case statements can be entered along multiple paths, either
2650-
// because they have multiple labels or because of fallthrough. When we
2651-
// need multiple entrance path, we factor the paths with a shared block.
2652-
if (!caseBlock->hasBoundDecls()) {
2653-
// Don't emit anything yet, we emit it at the cleanup level of the switch
2654-
// statement.
2655-
JumpDest sharedDest = emission.getSharedCaseBlockDest(caseBlock);
2656-
SGF.Cleanups.emitBranchAndCleanups(sharedDest, caseBlock);
2657-
return;
2658-
}
2659-
2660-
// If we don't have a fallthrough or a multi-pattern 'case', we can just
2661-
// emit the body inline and save some dead blocks. Emit the statement here.
2649+
// Certain case statements can be entered along multiple paths, either because
2650+
// they have multiple labels or because of fallthrough. When we need multiple
2651+
// entrance path, we factor the paths with a shared block. If we don't have a
2652+
// fallthrough or a multi-pattern 'case', we can just emit the body inline and
2653+
// save some dead blocks. Emit the statement here and bail early.
26622654
if (!row.hasFallthroughTo() && caseBlock->getCaseLabelItems().size() == 1) {
26632655
emission.emitCaseBody(caseBlock);
26642656
return;
26652657
}
26662658

2659+
// Ok, at this point we know that we have a multiple entrance block. Grab our
2660+
// shared destination in preperation for branching to it.
2661+
//
2662+
// NOTE: We do not emit anything yet, since we will emit the shared block
2663+
// later.
26672664
JumpDest sharedDest = emission.getSharedCaseBlockDest(caseBlock);
26682665

2669-
// Generate the arguments from this row's pattern in the case block's
2670-
// expected order, and keep those arguments from being cleaned up, as
2671-
// we're passing the +1 along to the shared case block dest. (The
2672-
// cleanups still happen, as they are threaded through here messily,
2673-
// but the explicit retains here counteract them, and then the
2674-
// retain/release pair gets optimized out.)
2666+
// If we do not have any bound decls, we do not need to setup any
2667+
// variables. Just jump to the shared destination.
2668+
if (!caseBlock->hasBoundDecls()) {
2669+
JumpDest sharedDest = emission.getSharedCaseBlockDest(caseBlock);
2670+
SGF.Cleanups.emitBranchAndCleanups(sharedDest, caseBlock);
2671+
return;
2672+
}
2673+
2674+
// Generate the arguments from this row's pattern in the case block's expected
2675+
// order, and keep those arguments from being cleaned up, as we're passing the
2676+
// +1 along to the shared case block dest. (The cleanups still happen, as they
2677+
// are threaded through here messily, but the explicit retains here counteract
2678+
// them, and then the retain/release pair gets optimized out.)
26752679
ArrayRef<CaseLabelItem> labelItems = caseBlock->getCaseLabelItems();
26762680
SmallVector<SILValue, 4> args;
26772681
SmallVector<VarDecl *, 4> expectedVarOrder;
@@ -2741,9 +2745,10 @@ void SILGenFunction::emitSwitchStmt(SwitchStmt *S) {
27412745
clauseRows.reserve(S->getRawCases().size());
27422746
bool hasFallthrough = false;
27432747
for (auto caseBlock : S->getCases()) {
2744-
if (!caseBlock->hasBoundDecls() ||
2745-
caseBlock->getCaseLabelItems().size() > 1 ||
2746-
hasFallthrough) {
2748+
// If the previous block falls through into this block or we have multiple
2749+
// case label itmes, create a shared case block to generate the shared
2750+
// block.
2751+
if (hasFallthrough || caseBlock->getCaseLabelItems().size() > 1) {
27472752
emission.initSharedCaseBlockDest(caseBlock, hasFallthrough);
27482753
}
27492754

‎test/SILGen/indirect_enum.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -229,9 +229,9 @@ func switchTreeB<T>(_ x: TreeB<T>) {
229229
switch x {
230230

231231
// CHECK: bb{{.*}}:
232+
// CHECK: function_ref @$s13indirect_enum1ayyF
232233
// CHECK: destroy_addr [[SCRATCH]]
233234
// CHECK: dealloc_stack [[SCRATCH]]
234-
// CHECK: function_ref @$s13indirect_enum1ayyF
235235
// CHECK: br [[OUTER_CONT:bb[0-9]+]]
236236
case .Nil:
237237
a()
@@ -305,9 +305,9 @@ func switchTreeB<T>(_ x: TreeB<T>) {
305305
// CHECK: br [[INNER_CONT:bb[0-9]+]]
306306

307307
// CHECK: [[INNER_CONT]]:
308+
// CHECK: function_ref @$s13indirect_enum1dyyF
308309
// CHECK: destroy_addr [[SCRATCH]]
309310
// CHECK: dealloc_stack [[SCRATCH]]
310-
// CHECK: function_ref @$s13indirect_enum1dyyF
311311
// CHECK: br [[OUTER_CONT]]
312312
default:
313313
d()

‎test/SILGen/switch.swift‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -304,11 +304,13 @@ func test_isa_1(p: P) {
304304
case is X:
305305
// CHECK: [[IS_X]]:
306306
// CHECK-NEXT: load [trivial] [[TMPBUF]]
307+
// CHECK-NEXT: // function_ref
308+
// CHECK-NEXT: [[FUNC:%.*]] = function_ref @$s6switch1ayyF
309+
// CHECK-NEXT: apply [[FUNC]]()
307310
// CHECK-NEXT: dealloc_stack [[TMPBUF]]
308311
// CHECK-NEXT: destroy_addr [[PTMPBUF]]
309312
// CHECK-NEXT: dealloc_stack [[PTMPBUF]]
310313
a()
311-
// CHECK: function_ref @$s6switch1ayyF
312314
// CHECK: br [[CONT:bb[0-9]+]]
313315

314316
// CHECK: [[IS_NOT_X]]:
@@ -423,9 +425,9 @@ func test_isa_class_1(x: B) {
423425

424426
// CHECK: [[YES_CASE1]]:
425427
case is D1 where runced():
428+
// CHECK: function_ref @$s6switch1ayyF
426429
// CHECK: destroy_value [[CAST_D1_COPY]]
427430
// CHECK: end_borrow [[CAST_D1]]
428-
// CHECK: function_ref @$s6switch1ayyF
429431
// CHECK: br [[CONT:bb[0-9]+]]
430432
a()
431433

@@ -443,8 +445,8 @@ func test_isa_class_1(x: B) {
443445
case is D2:
444446
// CHECK: [[IS_D2]]([[CAST_D2:%.*]] : @guaranteed $D2):
445447
// CHECK: [[CAST_D2_COPY:%.*]] = copy_value [[CAST_D2]]
446-
// CHECK: destroy_value [[CAST_D2_COPY]]
447448
// CHECK: function_ref @$s6switch1byyF
449+
// CHECK: destroy_value [[CAST_D2_COPY]]
448450
// CHECK: br [[CONT]]
449451
b()
450452

@@ -458,8 +460,8 @@ func test_isa_class_1(x: B) {
458460
// CHECK: cond_br {{%.*}}, [[CASE3:bb[0-9]+]], [[NO_CASE3:bb[0-9]+]]
459461

460462
// CHECK: [[CASE3]]:
461-
// CHECK: destroy_value [[CAST_E_COPY]]
462463
// CHECK: function_ref @$s6switch1cyyF
464+
// CHECK: destroy_value [[CAST_E_COPY]]
463465
// CHECK: br [[CONT]]
464466
c()
465467

@@ -478,9 +480,10 @@ func test_isa_class_1(x: B) {
478480
case is C:
479481
// CHECK: [[IS_C]]([[CAST_C:%.*]] : @guaranteed $C):
480482
// CHECK: [[CAST_C_COPY:%.*]] = copy_value [[CAST_C]]
483+
// CHECK: function_ref @$s6switch1dyyF
484+
// CHECK-NEXT: apply
481485
// CHECK: destroy_value [[CAST_C_COPY]]
482486
// CHECK: end_borrow [[CAST_C]]
483-
// CHECK: function_ref @$s6switch1dyyF
484487
// CHECK: br [[CONT]]
485488
d()
486489

@@ -799,25 +802,28 @@ func test_union_addr_only_1(u: MaybeAddressOnlyPair) {
799802
// CHECK: [[IS_LEFT]]:
800803
// CHECK: [[P:%.*]] = unchecked_take_enum_data_addr [[ENUM_ADDR]] : $*MaybeAddressOnlyPair, #MaybeAddressOnlyPair.Left!enumelt.1
801804
case .Left(_):
805+
// CHECK: [[FUNC:%.*]] = function_ref @$s6switch1byyF
806+
// CHECK-NEXT: apply [[FUNC]](
802807
// CHECK: destroy_addr [[P]]
803-
// CHECK: function_ref @$s6switch1byyF
804808
// CHECK: br [[CONT]]
805809
b()
806810

807811
// CHECK: [[IS_RIGHT]]:
808812
// CHECK: [[STR_ADDR:%.*]] = unchecked_take_enum_data_addr [[ENUM_ADDR]] : $*MaybeAddressOnlyPair, #MaybeAddressOnlyPair.Right!enumelt.1
809813
// CHECK: [[STR:%.*]] = load [take] [[STR_ADDR]]
810814
case .Right(_):
815+
// CHECK: [[FUNC:%.*]] = function_ref @$s6switch1cyyF
816+
// CHECK: apply [[FUNC]](
811817
// CHECK: destroy_value [[STR]] : $String
812-
// CHECK: function_ref @$s6switch1cyyF
813818
// CHECK: br [[CONT]]
814819
c()
815820

816821
// CHECK: [[IS_BOTH]]:
817822
// CHECK: [[P_STR_TUPLE:%.*]] = unchecked_take_enum_data_addr [[ENUM_ADDR]] : $*MaybeAddressOnlyPair, #MaybeAddressOnlyPair.Both!enumelt.1
818823
case .Both(_):
824+
// CHECK: [[FUNC:%.*]] = function_ref @$s6switch1dyyF
825+
// CHECK-NEXT: apply [[FUNC]](
819826
// CHECK: destroy_addr [[P_STR_TUPLE]]
820-
// CHECK: function_ref @$s6switch1dyyF
821827
// CHECK: br [[CONT]]
822828
d()
823829
}

‎test/SILGen/switch_var.swift‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -431,16 +431,17 @@ func test_let() {
431431
case bars():
432432
// CHECK: [[YES_CASE3]]:
433433
// CHECK: destroy_value [[VAL_COPY_3]]
434+
// CHECK: [[FUNC:%.*]] = function_ref @$s10switch_var1cyyF
435+
// CHECK-NEXT: apply [[FUNC]](
434436
// CHECK: destroy_value [[VAL]]
435-
// CHECK: function_ref @$s10switch_var1cyyF
436437
// CHECK: br [[CONT]]
437438
c()
438-
// CHECK: [[NO_CASE3]]:
439-
// CHECK: destroy_value [[VAL_COPY_3]]
440439

441440
case _:
442-
// CHECK: destroy_value [[VAL]]
441+
// CHECK: [[NO_CASE3]]:
442+
// CHECK: destroy_value [[VAL_COPY_3]]
443443
// CHECK: function_ref @$s10switch_var1dyyF
444+
// CHECK: destroy_value [[VAL]]
444445
// CHECK: br [[CONT]]
445446
d()
446447
}
@@ -504,18 +505,17 @@ func test_mixed_let_var() {
504505
case bars():
505506
// CHECK: [[CASE3]]:
506507
// CHECK: destroy_value [[VAL_COPY]]
507-
// CHECK: destroy_value [[VAL]]
508508
// CHECK: [[FUNC:%.*]] = function_ref @$s10switch_var1cyyF : $@convention(thin) () -> ()
509509
// CHECK: apply [[FUNC]]()
510+
// CHECK: destroy_value [[VAL]]
510511
// CHECK: br [[CONT]]
511512
c()
512513

513514
// CHECK: [[NOCASE3]]:
514515
// CHECK: destroy_value [[VAL_COPY]]
515-
516-
// CHECK: destroy_value [[VAL]]
517516
// CHECK: [[D_FUNC:%.*]] = function_ref @$s10switch_var1dyyF : $@convention(thin) () -> ()
518517
// CHECK: apply [[D_FUNC]]()
518+
// CHECK: destroy_value [[VAL]]
519519
// CHECK: br [[CONT]]
520520
case _:
521521
d()

0 commit comments

Comments
 (0)