Skip to content

Commit cd7c9e9

Browse files
gottesmmgottesmm
committed
[sil-combine] Fix cast optimizer based optimizations for ownership.
1 parent 4ff544b commit cd7c9e9

9 files changed

+248
-186
lines changed

‎lib/SILOptimizer/SILCombiner/SILCombinerCastVisitors.cpp

Lines changed: 51 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -567,13 +567,8 @@ static bool canBeUsedAsCastDestination(SILValue value, CastInst *castInst,
567567
DA->get(castInst->getFunction())->properlyDominates(value, castInst);
568568
}
569569

570-
571-
SILInstruction *
572-
SILCombiner::
573-
visitUnconditionalCheckedCastAddrInst(UnconditionalCheckedCastAddrInst *UCCAI) {
574-
if (UCCAI->getFunction()->hasOwnership())
575-
return nullptr;
576-
570+
SILInstruction *SILCombiner::visitUnconditionalCheckedCastAddrInst(
571+
UnconditionalCheckedCastAddrInst *uccai) {
577572
// Optimize the unconditional_checked_cast_addr in this pattern:
578573
//
579574
// %box = alloc_existential_box $Error, $ConcreteError
@@ -593,20 +588,36 @@ visitUnconditionalCheckedCastAddrInst(UnconditionalCheckedCastAddrInst *UCCAI) {
593588
//
594589
// This lets the alloc_existential_box become dead and it can be removed in
595590
// following optimizations.
596-
SILValue val = getConcreteValueOfExistentialBoxAddr(UCCAI->getSrc(), UCCAI);
597-
if (canBeUsedAsCastDestination(val, UCCAI, DA)) {
598-
SILBuilderContext builderCtx(Builder.getModule(), Builder.getTrackingList());
599-
SILBuilderWithScope builder(UCCAI, builderCtx);
600-
SILLocation loc = UCCAI->getLoc();
601-
builder.createRetainValue(loc, val, builder.getDefaultAtomicity());
602-
builder.createDestroyAddr(loc, UCCAI->getSrc());
603-
builder.createStore(loc, val, UCCAI->getDest(),
604-
StoreOwnershipQualifier::Unqualified);
605-
return eraseInstFromFunction(*UCCAI);
591+
SILValue val = getConcreteValueOfExistentialBoxAddr(uccai->getSrc(), uccai);
592+
while (auto *cvi = dyn_cast_or_null<CopyValueInst>(val))
593+
val = cvi->getOperand();
594+
if (canBeUsedAsCastDestination(val, uccai, DA)) {
595+
// We need to copy the value at its insertion point.
596+
{
597+
auto *valInsertPt = val->getDefiningInsertionPoint();
598+
if (!valInsertPt)
599+
return nullptr;
600+
auto valInsertPtIter = valInsertPt->getIterator();
601+
// If our value is defined by an instruction (not an argument), we want to
602+
// insert the copy after that. Otherwise, we have an argument and we want
603+
// to insert the copy right at the beginning of the block.
604+
if (val->getDefiningInstruction())
605+
valInsertPtIter = std::next(valInsertPtIter);
606+
SILBuilderWithScope builder(valInsertPtIter, Builder);
607+
val = builder.emitCopyValueOperation(valInsertPtIter->getLoc(), val);
608+
}
609+
610+
// Then we insert the destroy value/store at the cast location.
611+
SILBuilderWithScope builder(uccai, Builder);
612+
SILLocation loc = uccai->getLoc();
613+
builder.createDestroyAddr(loc, uccai->getSrc());
614+
builder.emitStoreValueOperation(loc, val, uccai->getDest(),
615+
StoreOwnershipQualifier::Init);
616+
return eraseInstFromFunction(*uccai);
606617
}
607618

608619
// Perform the purly type-based cast optimization.
609-
if (CastOpt.optimizeUnconditionalCheckedCastAddrInst(UCCAI))
620+
if (CastOpt.optimizeUnconditionalCheckedCastAddrInst(uccai))
610621
MadeChange = true;
611622

612623
return nullptr;
@@ -615,9 +626,6 @@ visitUnconditionalCheckedCastAddrInst(UnconditionalCheckedCastAddrInst *UCCAI) {
615626
SILInstruction *
616627
SILCombiner::
617628
visitUnconditionalCheckedCastInst(UnconditionalCheckedCastInst *UCCI) {
618-
if (UCCI->getFunction()->hasOwnership())
619-
return nullptr;
620-
621629
if (CastOpt.optimizeUnconditionalCheckedCastInst(UCCI)) {
622630
MadeChange = true;
623631
return nullptr;
@@ -770,9 +778,6 @@ SILCombiner::visitObjCToThickMetatypeInst(ObjCToThickMetatypeInst *OCTTMI) {
770778

771779
SILInstruction *
772780
SILCombiner::visitCheckedCastBranchInst(CheckedCastBranchInst *CBI) {
773-
if (CBI->getFunction()->hasOwnership())
774-
return nullptr;
775-
776781
if (CastOpt.optimizeCheckedCastBranchInst(CBI))
777782
MadeChange = true;
778783

@@ -782,9 +787,6 @@ SILCombiner::visitCheckedCastBranchInst(CheckedCastBranchInst *CBI) {
782787
SILInstruction *
783788
SILCombiner::
784789
visitCheckedCastAddrBranchInst(CheckedCastAddrBranchInst *CCABI) {
785-
if (CCABI->getFunction()->hasOwnership())
786-
return nullptr;
787-
788790
// Optimize the checked_cast_addr_br in this pattern:
789791
//
790792
// %box = alloc_existential_box $Error, $ConcreteError
@@ -808,11 +810,27 @@ visitCheckedCastAddrBranchInst(CheckedCastAddrBranchInst *CCABI) {
808810
//
809811
// TODO: Also handle the WillFail case.
810812
SILValue val = getConcreteValueOfExistentialBoxAddr(CCABI->getSrc(), CCABI);
813+
while (auto *cvi = dyn_cast_or_null<CopyValueInst>(val))
814+
val = cvi->getOperand();
811815
if (canBeUsedAsCastDestination(val, CCABI, DA)) {
812-
SILBuilderContext builderCtx(Builder.getModule(), Builder.getTrackingList());
813-
SILBuilderWithScope builder(CCABI, builderCtx);
816+
// We need to insert the copy after the defining instruction of val or at
817+
// the top of the block if val is an argument.
818+
{
819+
auto *valInsertPt = val->getDefiningInsertionPoint();
820+
if (!valInsertPt)
821+
return nullptr;
822+
auto valInsertPtIter = valInsertPt->getIterator();
823+
// If our value is defined by an instruction (not an argument), we want to
824+
// insert the copy after that. Otherwise, we have an argument and we want
825+
// to insert the copy right at the beginning of the block.
826+
if (val->getDefiningInstruction())
827+
valInsertPtIter = std::next(valInsertPtIter);
828+
SILBuilderWithScope builder(valInsertPtIter, Builder);
829+
val = builder.emitCopyValueOperation(valInsertPtIter->getLoc(), val);
830+
}
831+
832+
SILBuilderWithScope builder(CCABI, Builder);
814833
SILLocation loc = CCABI->getLoc();
815-
builder.createRetainValue(loc, val, builder.getDefaultAtomicity());
816834
switch (CCABI->getConsumptionKind()) {
817835
case CastConsumptionKind::TakeAlways:
818836
case CastConsumptionKind::TakeOnSuccess:
@@ -823,9 +841,9 @@ visitCheckedCastAddrBranchInst(CheckedCastAddrBranchInst *CCABI) {
823841
case CastConsumptionKind::BorrowAlways:
824842
llvm_unreachable("BorrowAlways is not supported on addresses");
825843
}
826-
builder.createStore(loc, val, CCABI->getDest(),
827-
StoreOwnershipQualifier::Unqualified);
828-
844+
builder.emitStoreValueOperation(loc, val, CCABI->getDest(),
845+
StoreOwnershipQualifier::Init);
846+
829847
// Replace the cast with a constant conditional branch.
830848
// Don't just create an unconditional branch to not change the CFG in
831849
// SILCombine. SimplifyCFG will clean that up.

‎lib/SILOptimizer/Utils/CastOptimizer.cpp

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1177,6 +1177,11 @@ SILInstruction *CastOptimizer::optimizeCheckedCastAddrBranchInst(
11771177
if (isa<DeallocStackInst>(User) || User == Inst)
11781178
continue;
11791179
if (auto *SI = dyn_cast<StoreInst>(User)) {
1180+
if (SI->getOwnershipQualifier() == StoreOwnershipQualifier::Assign) {
1181+
// We do not handle [assign]
1182+
isLegal = false;
1183+
break;
1184+
}
11801185
if (!Store) {
11811186
Store = SI;
11821187
continue;
@@ -1213,8 +1218,10 @@ SILInstruction *CastOptimizer::optimizeCheckedCastAddrBranchInst(
12131218
OwnershipKind::Owned);
12141219
B.setInsertionPoint(SuccessBB->begin());
12151220
// Store the result
1216-
B.createStore(Loc, SuccessBB->getArgument(0), Dest,
1217-
StoreOwnershipQualifier::Unqualified);
1221+
B.emitStoreValueOperation(Loc, SuccessBB->getArgument(0), Dest,
1222+
StoreOwnershipQualifier::Trivial);
1223+
if (B.hasOwnership())
1224+
FailureBB->createPhiArgument(MI->getType(), OwnershipKind::None);
12181225
eraseInstAction(Inst);
12191226
return NewI;
12201227
}

‎lib/SILOptimizer/Utils/InstOptUtils.cpp

Lines changed: 77 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -701,41 +701,58 @@ SILValue swift::
701701
getConcreteValueOfExistentialBox(AllocExistentialBoxInst *existentialBox,
702702
SILInstruction *ignoreUser) {
703703
StoreInst *singleStore = nullptr;
704-
for (Operand *use : getNonDebugUses(existentialBox)) {
704+
SmallVector<Operand *, 32> worklist;
705+
SmallPtrSet<Operand *, 8> addedToWorklistPreviously;
706+
for (auto *use : getNonDebugUses(existentialBox)) {
707+
worklist.push_back(use);
708+
addedToWorklistPreviously.insert(use);
709+
}
710+
711+
while (!worklist.empty()) {
712+
auto *use = worklist.pop_back_val();
705713
SILInstruction *user = use->getUser();
706714
switch (user->getKind()) {
707-
case SILInstructionKind::StrongRetainInst:
708-
case SILInstructionKind::StrongReleaseInst:
709-
break;
710-
case SILInstructionKind::ProjectExistentialBoxInst: {
711-
auto *projectedAddr = cast<ProjectExistentialBoxInst>(user);
712-
for (Operand *addrUse : getNonDebugUses(projectedAddr)) {
713-
if (auto *store = dyn_cast<StoreInst>(addrUse->getUser())) {
714-
assert(store->getSrc() != projectedAddr &&
715-
"cannot store an address");
716-
// Bail if there are multiple stores.
717-
if (singleStore)
718-
return SILValue();
719-
singleStore = store;
720-
continue;
721-
}
722-
// If there are other users to the box value address then bail out.
723-
return SILValue();
715+
case SILInstructionKind::StrongRetainInst:
716+
case SILInstructionKind::StrongReleaseInst:
717+
case SILInstructionKind::DestroyValueInst:
718+
case SILInstructionKind::EndBorrowInst:
719+
break;
720+
case SILInstructionKind::CopyValueInst:
721+
case SILInstructionKind::BeginBorrowInst:
722+
// Look through copy_value, begin_borrow
723+
for (SILValue result : user->getResults())
724+
for (auto *transitiveUse : result->getUses())
725+
if (!addedToWorklistPreviously.insert(transitiveUse).second)
726+
worklist.push_back(use);
727+
break;
728+
case SILInstructionKind::ProjectExistentialBoxInst: {
729+
auto *projectedAddr = cast<ProjectExistentialBoxInst>(user);
730+
for (Operand *addrUse : getNonDebugUses(projectedAddr)) {
731+
if (auto *store = dyn_cast<StoreInst>(addrUse->getUser())) {
732+
assert(store->getSrc() != projectedAddr && "cannot store an address");
733+
// Bail if there are multiple stores.
734+
if (singleStore)
735+
return SILValue();
736+
singleStore = store;
737+
continue;
724738
}
725-
break;
739+
// If there are other users to the box value address then bail out.
740+
return SILValue();
726741
}
727-
case SILInstructionKind::BuiltinInst: {
728-
auto *builtin = cast<BuiltinInst>(user);
729-
if (KeepWillThrowCall ||
730-
builtin->getBuiltinInfo().ID != BuiltinValueKind::WillThrow) {
731-
return SILValue();
732-
}
733-
break;
742+
break;
743+
}
744+
case SILInstructionKind::BuiltinInst: {
745+
auto *builtin = cast<BuiltinInst>(user);
746+
if (KeepWillThrowCall ||
747+
builtin->getBuiltinInfo().ID != BuiltinValueKind::WillThrow) {
748+
return SILValue();
734749
}
735-
default:
736-
if (user != ignoreUser)
737-
return SILValue();
738-
break;
750+
break;
751+
}
752+
default:
753+
if (user != ignoreUser)
754+
return SILValue();
755+
break;
739756
}
740757
}
741758
if (!singleStore)
@@ -753,29 +770,42 @@ getConcreteValueOfExistentialBoxAddr(SILValue addr, SILInstruction *ignoreUser)
753770
for (Operand *stackUse : stackLoc->getUses()) {
754771
SILInstruction *stackUser = stackUse->getUser();
755772
switch (stackUser->getKind()) {
756-
case SILInstructionKind::DeallocStackInst:
757-
case SILInstructionKind::DebugValueAddrInst:
758-
case SILInstructionKind::LoadInst:
759-
break;
760-
case SILInstructionKind::StoreInst: {
761-
auto *store = cast<StoreInst>(stackUser);
762-
assert(store->getSrc() != stackLoc && "cannot store an address");
763-
// Bail if there are multiple stores.
764-
if (singleStackStore)
765-
return SILValue();
766-
singleStackStore = store;
767-
break;
768-
}
769-
default:
770-
if (stackUser != ignoreUser)
773+
case SILInstructionKind::DestroyAddrInst: {
774+
// Make sure the destroy_addr is the instruction before one of our
775+
// dealloc_stack insts and is directly on the stack location.
776+
auto next = std::next(stackUser->getIterator());
777+
if (auto *dsi = dyn_cast<DeallocStackInst>(next))
778+
if (dsi->getOperand() != stackLoc)
771779
return SILValue();
772-
break;
780+
break;
781+
}
782+
case SILInstructionKind::DeallocStackInst:
783+
case SILInstructionKind::DebugValueAddrInst:
784+
case SILInstructionKind::LoadInst:
785+
break;
786+
case SILInstructionKind::StoreInst: {
787+
auto *store = cast<StoreInst>(stackUser);
788+
assert(store->getSrc() != stackLoc && "cannot store an address");
789+
// Bail if there are multiple stores.
790+
if (singleStackStore)
791+
return SILValue();
792+
singleStackStore = store;
793+
break;
794+
}
795+
default:
796+
if (stackUser != ignoreUser)
797+
return SILValue();
798+
break;
773799
}
774800
}
775801
if (!singleStackStore)
776802
return SILValue();
777803

778-
auto *box = dyn_cast<AllocExistentialBoxInst>(singleStackStore->getSrc());
804+
// Look through copy value insts.
805+
SILValue val = singleStackStore->getSrc();
806+
while (auto *cvi = dyn_cast<CopyValueInst>(val))
807+
val = cvi->getOperand();
808+
auto *box = dyn_cast<AllocExistentialBoxInst>(val);
779809
if (!box)
780810
return SILValue();
781811

‎test/SILOptimizer/sil_combine.sil

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2942,13 +2942,17 @@ struct OtherError: Error {
29422942
var payload: AnyObject
29432943
}
29442944

2945-
// CHECK-LABEL: sil @unconditional_existential_box_cast
2945+
// CHECK-LABEL: sil @unconditional_existential_box_cast :
2946+
// CHECK-NOT: unconditional_checked_cast_addr
2947+
// CHECK: retain_value %0
2948+
// CHECK-NOT: unconditional_checked_cast_addr
29462949
// CHECK: [[E:%[0-9]+]] = alloc_stack $Error
2950+
// CHECK-NOT: unconditional_checked_cast_addr
29472951
// CHECK: [[T:%[0-9]+]] = alloc_stack $TestError
2948-
// CHECK-NEXT: retain_value %0
29492952
// CHECK-NEXT: destroy_addr [[E]]
29502953
// CHECK-NEXT: store %0 to [[T]]
29512954
// CHECK-NEXT: struct_element_addr
2955+
// CHECK-NOT: unconditional_checked_cast_addr
29522956
// CHECK: } // end sil function 'unconditional_existential_box_cast'
29532957
sil @unconditional_existential_box_cast : $@convention(thin) (@guaranteed TestError) -> @owned AnyObject {
29542958
bb0(%0 : $TestError):
@@ -3114,13 +3118,14 @@ bb0(%0 : $TestError):
31143118
return %11 : $AnyObject
31153119
}
31163120

3117-
// CHECK-LABEL: sil @unconditional_existential_box_cast_not_dominating
3121+
// CHECK-LABEL: sil @unconditional_existential_box_cast_not_dominating :
3122+
// CHECK: retain_value %0
31183123
// CHECK: [[E:%[0-9]+]] = alloc_stack $Error
31193124
// CHECK: [[T:%[0-9]+]] = alloc_stack $TestError
3120-
// CHECK-NEXT: retain_value %0
31213125
// CHECK-NEXT: destroy_addr [[E]]
31223126
// CHECK-NEXT: store %0 to [[T]]
31233127
// CHECK-NEXT: struct_element_addr
3128+
// CHECK-NOT: unconditional_checked_cast_addr
31243129
// CHECK: } // end sil function 'unconditional_existential_box_cast_not_dominating'
31253130
sil @unconditional_existential_box_cast_not_dominating : $@convention(thin) (@guaranteed TestError) -> @owned AnyObject {
31263131
bb0(%0 : $TestError):
@@ -3152,12 +3157,13 @@ bb4:
31523157
}
31533158

31543159
// CHECK-LABEL: sil @conditional_existential_box_cast
3160+
// CHECK: retain_value %0
31553161
// CHECK: [[E:%[0-9]+]] = alloc_stack $Error
31563162
// CHECK: [[T:%[0-9]+]] = alloc_stack $TestError
3157-
// CHECK-NEXT: retain_value %0
31583163
// CHECK-NEXT: store %0 to [[T]]
31593164
// CHECK-NEXT: [[I:%[0-9]+]] = integer_literal $Builtin.Int1, -1
31603165
// CHECK-NEXT: cond_br [[I]], bb1, bb2
3166+
// CHECK-NOT: checked_cast_addr_br
31613167
// CHECK: } // end sil function 'conditional_existential_box_cast'
31623168
sil @conditional_existential_box_cast : $@convention(thin) (@guaranteed TestError) -> @owned AnyObject {
31633169
bb0(%0 : $TestError):
@@ -3186,13 +3192,14 @@ bb2:
31863192
}
31873193

31883194
// CHECK-LABEL: sil @taking_conditional_existential_box_cast
3195+
// CHECK: retain_value %0
31893196
// CHECK: [[E:%[0-9]+]] = alloc_stack $Error
31903197
// CHECK: [[T:%[0-9]+]] = alloc_stack $TestError
3191-
// CHECK-NEXT: retain_value %0
31923198
// CHECK-NEXT: destroy_addr [[E]]
31933199
// CHECK-NEXT: store %0 to [[T]]
31943200
// CHECK-NEXT: [[I:%[0-9]+]] = integer_literal $Builtin.Int1, -1
31953201
// CHECK-NEXT: cond_br [[I]], bb1, bb2
3202+
// CHECK-NOT: checked_cast_addr_br
31963203
// CHECK: } // end sil function 'taking_conditional_existential_box_cast'
31973204
sil @taking_conditional_existential_box_cast : $@convention(thin) (@guaranteed TestError) -> @owned AnyObject {
31983205
bb0(%0 : $TestError):

0 commit comments

Comments
 (0)