Skip to content

Commit 02939d1

Browse files
atrickatrick
committed
Fix SILCombine metatype cast optimization to avoid extending lifetimes.
This cannot be correctly done as a SILCombine because it must create new instructions at a previous location. Move the optimization into CastOptimizer. Insert the new metatype instructions in the correct spot. And manually do the replaceAllUsesWith and eraseInstruction steps. Fixes <rdar://problem/46746188> crash in swift_getObjCClassFromObject. (cherry picked from commit 65c95dd)
1 parent c86283e commit 02939d1

File tree

4 files changed

+100
-35
lines changed

4 files changed

+100
-35
lines changed

‎include/swift/SILOptimizer/Utils/CastOptimizer.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,10 @@ class CastOptimizer {
129129
SILValue Dest, CanType Source,
130130
CanType Target, SILBasicBlock *SuccessBB,
131131
SILBasicBlock *FailureBB);
132+
133+
SILInstruction *
134+
optimizeMetatypeConversion(ConversionInst *MCI,
135+
MetatypeRepresentation Representation);
132136
};
133137

134138
} // namespace swift

‎lib/SILOptimizer/SILCombiner/SILCombinerCastVisitors.cpp

Lines changed: 20 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -289,16 +289,19 @@ SILCombiner::visitUncheckedRefCastAddrInst(UncheckedRefCastAddrInst *URCI) {
289289
SILInstruction *
290290
SILCombiner::
291291
visitUnconditionalCheckedCastAddrInst(UnconditionalCheckedCastAddrInst *UCCAI) {
292-
CastOpt.optimizeUnconditionalCheckedCastAddrInst(UCCAI);
292+
if (CastOpt.optimizeUnconditionalCheckedCastAddrInst(UCCAI))
293+
MadeChange = true;
294+
293295
return nullptr;
294296
}
295297

296298
SILInstruction *
297299
SILCombiner::
298300
visitUnconditionalCheckedCastInst(UnconditionalCheckedCastInst *UCCI) {
299-
if (CastOpt.optimizeUnconditionalCheckedCastInst(UCCI))
301+
if (CastOpt.optimizeUnconditionalCheckedCastInst(UCCI)) {
302+
MadeChange = true;
300303
return nullptr;
301-
304+
}
302305
// FIXME: rename from RemoveCondFails to RemoveRuntimeAsserts.
303306
if (RemoveCondFails) {
304307
auto LoweredTargetType = UCCI->getType();
@@ -388,32 +391,6 @@ visitUncheckedBitwiseCastInst(UncheckedBitwiseCastInst *UBCI) {
388391
return nullptr;
389392
}
390393

391-
/// Helper function for simplifying conversions between
392-
/// thick and objc metatypes.
393-
static SILInstruction *
394-
visitMetatypeConversionInst(SILBuilder &Builder, ConversionInst *MCI,
395-
MetatypeRepresentation Representation) {
396-
SILValue Op = MCI->getOperand(0);
397-
// Instruction has a proper target type already.
398-
SILType Ty = MCI->getType();
399-
auto MetatypeTy = Op->getType().getAs<AnyMetatypeType>();
400-
401-
if (MetatypeTy->getRepresentation() != Representation)
402-
return nullptr;
403-
404-
if (isa<MetatypeInst>(Op))
405-
return Builder.createMetatype(MCI->getLoc(), Ty);
406-
407-
if (auto *VMI = dyn_cast<ValueMetatypeInst>(Op))
408-
return Builder.createValueMetatype(MCI->getLoc(), Ty, VMI->getOperand());
409-
410-
if (auto *EMI = dyn_cast<ExistentialMetatypeInst>(Op))
411-
return Builder.createExistentialMetatype(MCI->getLoc(), Ty,
412-
EMI->getOperand());
413-
414-
return nullptr;
415-
}
416-
417394
SILInstruction *
418395
SILCombiner::visitThickToObjCMetatypeInst(ThickToObjCMetatypeInst *TTOCMI) {
419396
// Perform the following transformations:
@@ -425,8 +402,10 @@ SILCombiner::visitThickToObjCMetatypeInst(ThickToObjCMetatypeInst *TTOCMI) {
425402
//
426403
// (thick_to_objc_metatype (existential_metatype @thick)) ->
427404
// (existential_metatype @objc_metatype)
428-
return visitMetatypeConversionInst(Builder, TTOCMI,
429-
MetatypeRepresentation::Thick);
405+
if (CastOpt.optimizeMetatypeConversion(TTOCMI, MetatypeRepresentation::Thick))
406+
MadeChange = true;
407+
408+
return nullptr;
430409
}
431410

432411
SILInstruction *
@@ -440,20 +419,26 @@ SILCombiner::visitObjCToThickMetatypeInst(ObjCToThickMetatypeInst *OCTTMI) {
440419
//
441420
// (objc_to_thick_metatype (existential_metatype @objc_metatype)) ->
442421
// (existential_metatype @thick)
443-
return visitMetatypeConversionInst(Builder, OCTTMI,
444-
MetatypeRepresentation::ObjC);
422+
if (CastOpt.optimizeMetatypeConversion(OCTTMI, MetatypeRepresentation::ObjC))
423+
MadeChange = true;
424+
425+
return nullptr;
445426
}
446427

447428
SILInstruction *
448429
SILCombiner::visitCheckedCastBranchInst(CheckedCastBranchInst *CBI) {
449-
CastOpt.optimizeCheckedCastBranchInst(CBI);
430+
if (CastOpt.optimizeCheckedCastBranchInst(CBI))
431+
MadeChange = true;
432+
450433
return nullptr;
451434
}
452435

453436
SILInstruction *
454437
SILCombiner::
455438
visitCheckedCastAddrBranchInst(CheckedCastAddrBranchInst *CCABI) {
456-
CastOpt.optimizeCheckedCastAddrBranchInst(CCABI);
439+
if (CastOpt.optimizeCheckedCastAddrBranchInst(CCABI))
440+
MadeChange = true;
441+
457442
return nullptr;
458443
}
459444

‎lib/SILOptimizer/Utils/CastOptimizer.cpp

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1576,3 +1576,40 @@ SILInstruction *CastOptimizer::optimizeUnconditionalCheckedCastAddrInst(
15761576

15771577
return nullptr;
15781578
}
1579+
1580+
/// Simplify conversions between thick and objc metatypes.
1581+
SILInstruction *CastOptimizer::optimizeMetatypeConversion(
1582+
ConversionInst *MCI, MetatypeRepresentation Representation) {
1583+
SILValue Op = MCI->getOperand(0);
1584+
// Instruction has a proper target type already.
1585+
SILType Ty = MCI->getType();
1586+
auto MetatypeTy = Op->getType().getAs<AnyMetatypeType>();
1587+
1588+
if (MetatypeTy->getRepresentation() != Representation)
1589+
return nullptr;
1590+
1591+
// Rematerialize the incoming metatype instruction with the outgoing type.
1592+
auto replaceCast = [&](SingleValueInstruction *NewCast) {
1593+
assert(Ty.getAs<AnyMetatypeType>()->getRepresentation()
1594+
== NewCast->getType().getAs<AnyMetatypeType>()->getRepresentation());
1595+
MCI->replaceAllUsesWith(NewCast);
1596+
EraseInstAction(MCI);
1597+
return NewCast;
1598+
};
1599+
if (auto *MI = dyn_cast<MetatypeInst>(Op)) {
1600+
return replaceCast(
1601+
SILBuilderWithScope(MCI).createMetatype(MCI->getLoc(), Ty));
1602+
}
1603+
// For metatype instructions that require an operand, generate the new
1604+
// metatype at the same position as the original to avoid extending the
1605+
// lifetime of `Op` past its destroy.
1606+
if (auto *VMI = dyn_cast<ValueMetatypeInst>(Op)) {
1607+
return replaceCast(SILBuilderWithScope(VMI).createValueMetatype(
1608+
MCI->getLoc(), Ty, VMI->getOperand()));
1609+
}
1610+
if (auto *EMI = dyn_cast<ExistentialMetatypeInst>(Op)) {
1611+
return replaceCast(SILBuilderWithScope(EMI).createExistentialMetatype(
1612+
MCI->getLoc(), Ty, EMI->getOperand()));
1613+
}
1614+
return nullptr;
1615+
}

‎test/SILOptimizer/sil_combine.sil

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3673,3 +3673,42 @@ bb0(%0 : $Builtin.Int64):
36733673
return %4 : $Builtin.Int64
36743674
}
36753675

3676+
// <rdar://46746188> crash in swift_getObjCClassFromObject
3677+
// class TestDocument: NSPersistentDocument {
3678+
// override init() {
3679+
// super.init()
3680+
// }
3681+
// convenience init(type: String) throws {
3682+
// self.init()
3683+
// }
3684+
// }
3685+
// After inlining the __allocating_init, we have two uses of the value_metatype.
3686+
// The second use goes through a thick_to_objc_metatype. When SILCombine replaces
3687+
// thick_to_objc_metatype with value_metatype, it cannot extend the liverange
3688+
// of value_metatype's operand.
3689+
@objc class TestObjCInit {
3690+
init()
3691+
3692+
convenience init(type: String) throws
3693+
}
3694+
// CHECK-LABEL: sil hidden [thunk] @objc_init_partial_dealloc : $@convention(objc_method) (@owned TestObjCInit) -> Optional<TestObjCInit> {
3695+
// CHECK: bb0(%0 : $TestObjCInit):
3696+
// CHECK: [[VMT2:%.*]] = value_metatype $@objc_metatype TestObjCInit.Type, %0 : $TestObjCInit
3697+
// CHECK: [[VMT:%.*]] = value_metatype $@thick TestObjCInit.Type, %0 : $TestObjCInit
3698+
// CHECK: dealloc_partial_ref %0 : $TestObjCInit, [[VMT]] : $@thick TestObjCInit.Type
3699+
// CHECK-NOT: value_metatype
3700+
// CHECK: [[O:%.*]] = alloc_ref_dynamic [objc] [[VMT2]] : $@objc_metatype TestObjCInit.Type, $TestObjCInit
3701+
// CHECK: [[M:%.*]] = objc_method [[O]] : $TestObjCInit, #TestObjCInit.init!initializer.1.foreign : (TestObjCInit.Type) -> () -> TestObjCInit, $@convention(objc_method) (@owned TestObjCInit) -> @owned TestObjCInit
3702+
// CHECK: apply [[M]]([[O]]) : $@convention(objc_method) (@owned TestObjCInit) -> @owned TestObjCInit
3703+
// CHECK-LABEL: } // end sil function 'objc_init_partial_dealloc'
3704+
sil hidden [thunk] @objc_init_partial_dealloc : $@convention(objc_method) (@owned TestObjCInit) -> Optional<TestObjCInit> {
3705+
bb0(%2 : $TestObjCInit):
3706+
%8 = value_metatype $@thick TestObjCInit.Type, %2 : $TestObjCInit
3707+
dealloc_partial_ref %2 : $TestObjCInit, %8 : $@thick TestObjCInit.Type
3708+
%11 = thick_to_objc_metatype %8 : $@thick TestObjCInit.Type to $@objc_metatype TestObjCInit.Type
3709+
%12 = alloc_ref_dynamic [objc] %11 : $@objc_metatype TestObjCInit.Type, $TestObjCInit
3710+
%13 = objc_method %12 : $TestObjCInit, #TestObjCInit.init!initializer.1.foreign : (TestObjCInit.Type) -> () -> TestObjCInit, $@convention(objc_method) (@owned TestObjCInit) -> @owned TestObjCInit
3711+
%14 = apply %13(%12) : $@convention(objc_method) (@owned TestObjCInit) -> @owned TestObjCInit
3712+
%19 = enum $Optional<TestObjCInit>, #Optional.some!enumelt.1, %14 : $TestObjCInit
3713+
return %19 : $Optional<TestObjCInit>
3714+
}

0 commit comments

Comments
 (0)