Skip to content

Commit 5d28fbd

Browse files
committed
SILCombiner: Use a substitution list with concrete type if we can cast the apply operand
This works around an issue where using an apply with an unsubstituted substitution map causes issues in downstream optimizations. ``` %9 = alloc_stack $@opened("60E354F4-17B9-11EB-9427-ACDE48001122") NonClassProto copy_addr %8 to [initialization] %9 : $*@opened("60E354F4-17B9-11EB-9427-ACDE48001122") NonClassProto %11 = witness_method $ConformerClass, #NonClassProto.myVariable!getter : <Self where Self : NonClassProto> (Self) -> () -> SomeValue : $@convention(witness_method: NonClassProto) <τ_0_0 where τ_0_0 : NonClassProto> (@in_guaranteed τ_0_0) -> SomeValue apply %11<@opened("60E354F4-17B9-11EB-9427-ACDE48001122") NonClassProto>(%9) : $@convention(witness_method: NonClassProto) <τ_0_0 where τ_0_0 : NonClassProto> (@in_guaranteed τ_0_0) -> SomeValue ``` The problem arise when the devirtualizer replace `witness_method $ConformerClass, #NonClassProto.myVariable!getter` with the underlying implementation. That implementation for better or worse is further constrained to `Self : ConformerClass` and applying an opened existential which is not class constraint is a recipe for disaster. The proper solution would probably be for the devirtualizer to insert the cast if necessary and update the substitution list. That fix will be left for another day though. rdar://70582785
1 parent d32a371 commit 5d28fbd

File tree

6 files changed

+115
-7
lines changed

6 files changed

+115
-7
lines changed

lib/SILOptimizer/SILCombiner/SILCombiner.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -291,6 +291,8 @@ class SILCombiner :
291291

292292
bool canReplaceArg(FullApplySite Apply, const OpenedArchetypeInfo &OAI,
293293
const ConcreteExistentialInfo &CEI, unsigned ArgIdx);
294+
SILValue canCastArg(FullApplySite Apply, const OpenedArchetypeInfo &OAI,
295+
const ConcreteExistentialInfo &CEI, unsigned ArgIdx);
294296

295297
SILInstruction *createApplyWithConcreteType(
296298
FullApplySite Apply,

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -994,6 +994,48 @@ struct ConcreteArgumentCopy {
994994
}
995995
};
996996

997+
SILValue SILCombiner::canCastArg(FullApplySite Apply,
998+
const OpenedArchetypeInfo &OAI,
999+
const ConcreteExistentialInfo &CEI,
1000+
unsigned ArgIdx) {
1001+
if (!CEI.ConcreteValue || CEI.ConcreteType->isOpenedExistential() ||
1002+
!CEI.ConcreteValue->getType().isAddress())
1003+
return SILValue();
1004+
1005+
// Don't specialize apply instructions that return the callee's Arg type,
1006+
// because this optimization does not know how to substitute types in the
1007+
// users of this apply. In the function type substitution below, all
1008+
// references to OpenedArchetype will be substituted. So walk to type to
1009+
// find all possible references, such as returning Optional<Arg>.
1010+
if (Apply.getType().getASTType().findIf(
1011+
[&OAI](Type t) -> bool { return t->isEqual(OAI.OpenedArchetype); })) {
1012+
return SILValue();
1013+
}
1014+
// Bail out if any other arguments or indirect result that refer to the
1015+
// OpenedArchetype. The following optimization substitutes all occurrences
1016+
// of OpenedArchetype in the function signature, but will only rewrite the
1017+
// Arg operand.
1018+
//
1019+
// Note that the language does not allow Self to occur in contravariant
1020+
// position. However, SIL does allow this and it can happen as a result of
1021+
// upstream transformations. Since this is bail-out logic, it must handle
1022+
// all verifiable SIL.
1023+
1024+
// This bailout check is also needed for non-Self arguments [including Self].
1025+
unsigned NumApplyArgs = Apply.getNumArguments();
1026+
for (unsigned Idx = 0; Idx < NumApplyArgs; ++Idx) {
1027+
if (Idx == ArgIdx)
1028+
continue;
1029+
if (Apply.getArgument(Idx)->getType().getASTType().findIf(
1030+
[&OAI](Type t) -> bool {
1031+
return t->isEqual(OAI.OpenedArchetype);
1032+
})) {
1033+
return SILValue();
1034+
}
1035+
}
1036+
return Builder.createUncheckedAddrCast(
1037+
Apply.getLoc(), Apply.getArgument(ArgIdx), CEI.ConcreteValue->getType());
1038+
}
9971039
/// Rewrite the given method apply instruction in terms of the provided conrete
9981040
/// type information.
9991041
///
@@ -1049,6 +1091,32 @@ SILInstruction *SILCombiner::createApplyWithConcreteType(
10491091

10501092
// Check for Arg's concrete type propagation legality.
10511093
if (!canReplaceArg(Apply, OAI, CEI, ArgIdx)) {
1094+
1095+
// As on last fall-back try to cast the argument.
1096+
if (auto cast = canCastArg(Apply, OAI, CEI, ArgIdx)) {
1097+
NewArgs.push_back(cast);
1098+
// Form a new set of substitutions where the argument is
1099+
// replaced with a concrete type.
1100+
NewCallSubs = NewCallSubs.subst(
1101+
[&](SubstitutableType *type) -> Type {
1102+
if (type == OAI.OpenedArchetype)
1103+
return CEI.ConcreteType;
1104+
return type;
1105+
},
1106+
[&](CanType origTy, Type substTy,
1107+
ProtocolDecl *proto) -> ProtocolConformanceRef {
1108+
if (origTy->isEqual(OAI.OpenedArchetype)) {
1109+
assert(substTy->isEqual(CEI.ConcreteType));
1110+
// Do a conformance lookup on this witness requirement using the
1111+
// existential's conformances. The witness requirement may be a
1112+
// base type of the existential's requirements.
1113+
return CEI.lookupExistentialConformance(proto);
1114+
}
1115+
return ProtocolConformanceRef(proto);
1116+
});
1117+
continue;
1118+
}
1119+
// Otherwise, use the original argument.
10521120
NewArgs.push_back(Apply.getArgument(ArgIdx));
10531121
continue;
10541122
}

test/SILOptimizer/sil_combine_apply.sil

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,8 @@ bb0(%0 : $*MutatingProto, %1 : $MStruct):
369369
%11 = open_existential_addr mutable_access %9 : $*MutatingProto to $*@opened("FC5F3CFA-A7A4-11E7-911F-685B35C48C83") MutatingProto
370370
// CHECK: [[M:%[0-9]+]] = witness_method $MStruct,
371371
%12 = witness_method $@opened("FC5F3CFA-A7A4-11E7-911F-685B35C48C83") MutatingProto, #MutatingProto.mutatingMethod : <Self where Self : MutatingProto> (inout Self) -> () -> (), %11 : $*@opened("FC5F3CFA-A7A4-11E7-911F-685B35C48C83") MutatingProto : $@convention(witness_method: MutatingProto) <τ_0_0 where τ_0_0 : MutatingProto> (@inout τ_0_0) -> ()
372-
// CHECK: apply [[M]]<@opened("{{.*}}") MutatingProto>([[E]]) :
372+
// CHECK: [[C:%[0-9]+]] = unchecked_addr_cast [[E]] : $*@opened("FC5F3CFA-A7A4-11E7-911F-685B35C48C83") MutatingProto to $*MStruct
373+
// CHECK: apply [[M]]<MStruct>([[C]]) :
373374
%13 = apply %12<@opened("FC5F3CFA-A7A4-11E7-911F-685B35C48C83") MutatingProto>(%11) : $@convention(witness_method: MutatingProto) <τ_0_0 where τ_0_0 : MutatingProto> (@inout τ_0_0) -> ()
374375
copy_addr [take] %9 to [initialization] %0 : $*MutatingProto
375376
dealloc_stack %9 : $*MutatingProto

test/SILOptimizer/sil_combine_concrete_existential.sil

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -470,7 +470,8 @@ sil [transparent] [reabstraction_thunk] @testDevirtExistentialEnumThunk : $@conv
470470
// CHECK: [[THUNK:%.*]] = function_ref @testDevirtExistentialEnumThunk : $@convention(thin) (UnsafeRawBufferPointer) -> (@out (), @error Error)
471471
// CHECK: [[TTF:%.*]] = thin_to_thick_function [[THUNK:%.*]] : $@convention(thin) (UnsafeRawBufferPointer) -> (@out (), @error Error) to $@noescape @callee_guaranteed (UnsafeRawBufferPointer) -> (@out (), @error Error)
472472
// CHECK: [[WM:%.*]] = witness_method $Array<Int>, #ContiguousBytes.withUnsafeBytes : <Self where Self : ContiguousBytes><R> (Self) -> ((UnsafeRawBufferPointer) throws -> R) throws -> R : $@convention(witness_method: ContiguousBytes) <τ_0_0 where τ_0_0 : ContiguousBytes><τ_1_0> (@noescape @callee_guaranteed (UnsafeRawBufferPointer) -> (@out τ_1_0, @error Error), @in_guaranteed τ_0_0) -> (@out τ_1_0, @error Error)
473-
// CHECK: apply [nothrow] [[WM]]<@opened("8402EC1A-F35D-11E8-950A-D0817AD9F6DD") ContiguousBytes, ()>(%{{.*}}, [[TTF]], [[OPENED]]) : $@convention(witness_method: ContiguousBytes) <τ_0_0 where τ_0_0 : ContiguousBytes><τ_1_0> (@noescape @callee_guaranteed (UnsafeRawBufferPointer) -> (@out τ_1_0, @error Error), @in_guaranteed τ_0_0) -> (@out τ_1_0, @error Error)
473+
// CHECK: [[CAST:%.*]] = unchecked_addr_cast [[OPENED]]
474+
// CHECK: apply [nothrow] [[WM]]<Array<Int>, ()>(%{{.*}}, [[TTF]], [[CAST]]) : $@convention(witness_method: ContiguousBytes) <τ_0_0 where τ_0_0 : ContiguousBytes><τ_1_0> (@noescape @callee_guaranteed (UnsafeRawBufferPointer) -> (@out τ_1_0, @error Error), @in_guaranteed τ_0_0) -> (@out τ_1_0, @error Error)
474475
// CHECK: destroy_addr [[ALLOC_EXISTENTIAL]] : $*ContiguousBytes
475476
// CHECK-LABEL: } // end sil function 'testDevirtExistentialEnum'
476477
sil shared [noinline] @testDevirtExistentialEnum : $@convention(thin) (@guaranteed Array<Int>) -> () {

test/SILOptimizer/sil_combine_protocol_conf.swift

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -356,3 +356,37 @@ internal class OtherKlass {
356356
}
357357
}
358358

359+
// Don't assert on this following example.
360+
361+
public struct SomeValue {}
362+
363+
public protocol MyVariableProtocol : class {
364+
}
365+
366+
extension MyVariableProtocol where Self: MyBaseClass {
367+
@inline(never)
368+
public var myVariable : SomeValue {
369+
print(self)
370+
return SomeValue()
371+
}
372+
}
373+
public class MyBaseClass : MyVariableProtocol {}
374+
375+
fileprivate protocol NonClassProto {
376+
var myVariable : SomeValue { get }
377+
}
378+
379+
380+
fileprivate class ConformerClass : MyBaseClass, NonClassProto {}
381+
382+
383+
@inline(never)
384+
private func repo(_ c: NonClassProto) {
385+
let p : NonClassProto = c
386+
print(p.myVariable)
387+
}
388+
389+
public func entryPoint() {
390+
let c = ConformerClass()
391+
repo(c)
392+
}

test/SILOptimizer/sil_combiner_concrete_prop_all_args.sil

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -451,9 +451,10 @@ bb0(%0 : $*PPP):
451451
// CHECK: bb0:
452452
// CHECK: [[ALLOC:%.*]] = alloc_stack $PPP, var, name "magic6"
453453
// CHECK: alloc_ref $KKK
454-
// CHECK: [[F:%.*]] = function_ref @$s21existential_transform14wrap_inout_ncp1as5Int32VAA3PPP_pz_tFTf4e_n : $@convention(thin) <τ_0_0 where τ_0_0 : PPP> (@inout τ_0_0) -> Int32
455454
// CHECK: [[ADR:%.*]] = open_existential_addr mutable_access [[ALLOC]] : $*PPP to $*@opened("{{.*}}") PPP
456-
// CHECK: apply [[F]]<@opened("{{.*}}") PPP>([[ADR]]) : $@convention(thin) <τ_0_0 where τ_0_0 : PPP> (@inout τ_0_0) -> Int32
455+
// CHECK: [[CAST:%.*]] = unchecked_addr_cast [[ADR]]
456+
// CHECK: [[F:%.*]] = function_ref @$s21existential_transform14wrap_inout_ncp1as5Int32VAA3PPP_pz_tFTf4e_n4main3KKKC_Tg5 : $@convention(thin) (@inout KKK) -> Int32
457+
// CHECK: apply [[F]]([[CAST]])
457458
// CHECK: dealloc_stack %0 : $*PPP
458459
// CHECK-LABEL: } // end sil function '$s21existential_transform9inout_ncpyyF'
459460
sil hidden [noinline] @$s21existential_transform9inout_ncpyyF : $@convention(thin) () -> () {
@@ -500,9 +501,10 @@ bb0(%0 : $*PPPP):
500501
// CHECK: bb0:
501502
// CHECK: [[ALLOC:%.*]] = alloc_stack $PPPP, var, name "magic7"
502503
// CHECK: struct $SSSS ()
503-
// CHECK: [[F:%.*]] = function_ref @$s21existential_transform21wrap_struct_inout_ncp1as5Int32VAA4PPPP_pz_tFTf4e_n : $@convention(thin) <τ_0_0 where τ_0_0 : PPPP> (@inout τ_0_0) -> Int32
504-
// CHECK: [[ADR:%.*]] = open_existential_addr mutable_access %0 : $*PPPP to $*@opened("{{.*}}") PPPP
505-
// CHECK: apply [[F]]<@opened("{{.*}}") PPPP>(%5) : $@convention(thin) <τ_0_0 where τ_0_0 : PPPP> (@inout τ_0_0) -> Int32
504+
// CHECK: [[ADR:%.*]] = open_existential_addr mutable_access %0 : $*PPPP to $*@opened("{{.*}}") PPPP
505+
// CHECK: [[CAST:%.*]] = unchecked_addr_cast [[ADR]]
506+
// CHECK: [[F:%.*]] = function_ref @$s21existential_transform21wrap_struct_inout_ncp1as5Int32VAA4PPPP_pz_tFTf4e_n4main4SSSSV_Tg5 : $@convention(thin) (@inout SSSS) -> Int32
507+
// CHECK: apply [[F]]([[CAST]])
506508
// CHECK: dealloc_stack [[ALLOC]] : $*PPPP
507509
// CHECK-LABEL: } // end sil function '$s21existential_transform16struct_inout_ncpyyF'
508510
sil hidden [noinline] @$s21existential_transform16struct_inout_ncpyyF : $@convention(thin) () -> () {

0 commit comments

Comments
 (0)