Skip to content

Commit 65c95dd

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.
1 parent 14dfa75 commit 65c95dd

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
@@ -1575,3 +1575,40 @@ SILInstruction *CastOptimizer::optimizeUnconditionalCheckedCastAddrInst(
15751575

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

‎test/SILOptimizer/sil_combine.sil

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3575,3 +3575,42 @@ bb0(%0 : $Builtin.Int64):
35753575
return %4 : $Builtin.Int64
35763576
}
35773577

3578+
// <rdar://46746188> crash in swift_getObjCClassFromObject
3579+
// class TestDocument: NSPersistentDocument {
3580+
// override init() {
3581+
// super.init()
3582+
// }
3583+
// convenience init(type: String) throws {
3584+
// self.init()
3585+
// }
3586+
// }
3587+
// After inlining the __allocating_init, we have two uses of the value_metatype.
3588+
// The second use goes through a thick_to_objc_metatype. When SILCombine replaces
3589+
// thick_to_objc_metatype with value_metatype, it cannot extend the liverange
3590+
// of value_metatype's operand.
3591+
@objc class TestObjCInit {
3592+
init()
3593+
3594+
convenience init(type: String) throws
3595+
}
3596+
// CHECK-LABEL: sil hidden [thunk] @objc_init_partial_dealloc : $@convention(objc_method) (@owned TestObjCInit) -> Optional<TestObjCInit> {
3597+
// CHECK: bb0(%0 : $TestObjCInit):
3598+
// CHECK: [[VMT2:%.*]] = value_metatype $@objc_metatype TestObjCInit.Type, %0 : $TestObjCInit
3599+
// CHECK: [[VMT:%.*]] = value_metatype $@thick TestObjCInit.Type, %0 : $TestObjCInit
3600+
// CHECK: dealloc_partial_ref %0 : $TestObjCInit, [[VMT]] : $@thick TestObjCInit.Type
3601+
// CHECK-NOT: value_metatype
3602+
// CHECK: [[O:%.*]] = alloc_ref_dynamic [objc] [[VMT2]] : $@objc_metatype TestObjCInit.Type, $TestObjCInit
3603+
// CHECK: [[M:%.*]] = objc_method [[O]] : $TestObjCInit, #TestObjCInit.init!initializer.1.foreign : (TestObjCInit.Type) -> () -> TestObjCInit, $@convention(objc_method) (@owned TestObjCInit) -> @owned TestObjCInit
3604+
// CHECK: apply [[M]]([[O]]) : $@convention(objc_method) (@owned TestObjCInit) -> @owned TestObjCInit
3605+
// CHECK-LABEL: } // end sil function 'objc_init_partial_dealloc'
3606+
sil hidden [thunk] @objc_init_partial_dealloc : $@convention(objc_method) (@owned TestObjCInit) -> Optional<TestObjCInit> {
3607+
bb0(%2 : $TestObjCInit):
3608+
%8 = value_metatype $@thick TestObjCInit.Type, %2 : $TestObjCInit
3609+
dealloc_partial_ref %2 : $TestObjCInit, %8 : $@thick TestObjCInit.Type
3610+
%11 = thick_to_objc_metatype %8 : $@thick TestObjCInit.Type to $@objc_metatype TestObjCInit.Type
3611+
%12 = alloc_ref_dynamic [objc] %11 : $@objc_metatype TestObjCInit.Type, $TestObjCInit
3612+
%13 = objc_method %12 : $TestObjCInit, #TestObjCInit.init!initializer.1.foreign : (TestObjCInit.Type) -> () -> TestObjCInit, $@convention(objc_method) (@owned TestObjCInit) -> @owned TestObjCInit
3613+
%14 = apply %13(%12) : $@convention(objc_method) (@owned TestObjCInit) -> @owned TestObjCInit
3614+
%19 = enum $Optional<TestObjCInit>, #Optional.some!enumelt.1, %14 : $TestObjCInit
3615+
return %19 : $Optional<TestObjCInit>
3616+
}

0 commit comments

Comments
 (0)