Skip to content

Commit 89ce5b1

Browse files
tkremenektkremenek
authored
Merge pull request #4641 from swiftix/swift-3.0-branch-rdar-28136015
[sil-cse] Fix a bug in the CSE of open_existential_ref instructions
2 parents 14df2a5 + ec7acc6 commit 89ce5b1

File tree

7 files changed

+193
-145
lines changed

7 files changed

+193
-145
lines changed

‎include/swift/SIL/SILBuilder.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -954,8 +954,9 @@ class SILBuilder {
954954
DynamicMethodInst *createDynamicMethod(SILLocation Loc, SILValue Operand,
955955
SILDeclRef Member, SILType MethodTy,
956956
bool Volatile = false) {
957-
return insert(new (F.getModule()) DynamicMethodInst(
958-
getSILDebugLocation(Loc), Operand, Member, MethodTy, Volatile));
957+
return insert(DynamicMethodInst::create(
958+
getSILDebugLocation(Loc), Operand, Member, MethodTy, Volatile,
959+
&F, OpenedArchetypes));
959960
}
960961

961962
OpenExistentialAddrInst *

‎include/swift/SIL/SILCloner.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,7 @@ class SILBuilderWithPostProcess : public SILBuilder {
270270
{
271271
setInsertionPoint(SC.getBuilder().getInsertionBB(),
272272
SC.getBuilder().getInsertionPoint());
273+
setOpenedArchetypesTracker(SC.getBuilder().getOpenedArchetypesTracker());
273274
}
274275

275276
~SILBuilderWithPostProcess() {

‎include/swift/SIL/SILInstruction.h

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3613,14 +3613,25 @@ class WitnessMethodInst final
36133613
/// constant referring to some Objective-C method, performs dynamic method
36143614
/// lookup to extract the implementation of that method. This method lookup
36153615
/// can fail at run-time
3616-
class DynamicMethodInst
3617-
: public UnaryInstructionBase<ValueKind::DynamicMethodInst, MethodInst>
3616+
class DynamicMethodInst final
3617+
: public UnaryInstructionWithOpenArchetypesBase<
3618+
ValueKind::DynamicMethodInst,
3619+
DynamicMethodInst,
3620+
MethodInst,
3621+
true>
36183622
{
36193623
friend class SILBuilder;
36203624

36213625
DynamicMethodInst(SILDebugLocation DebugLoc, SILValue Operand,
3622-
SILDeclRef Member, SILType Ty, bool Volatile = false)
3623-
: UnaryInstructionBase(DebugLoc, Operand, Ty, Member, Volatile) {}
3626+
ArrayRef<SILValue> TypeDependentOperands,
3627+
SILDeclRef Member, SILType Ty, bool Volatile)
3628+
: UnaryInstructionWithOpenArchetypesBase(DebugLoc, Operand,
3629+
TypeDependentOperands, Ty, Member, Volatile) {}
3630+
3631+
static DynamicMethodInst *
3632+
create(SILDebugLocation DebugLoc, SILValue Operand,
3633+
SILDeclRef Member, SILType Ty, bool Volatile, SILFunction *F,
3634+
SILOpenedArchetypesState &OpenedArchetypes);
36243635
};
36253636

36263637
/// Given the address of an existential, "opens" the

‎lib/SIL/SILInstructions.cpp

Lines changed: 82 additions & 101 deletions
Large diffs are not rendered by default.

‎lib/SIL/SILVerifier.cpp

Lines changed: 38 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -698,18 +698,16 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
698698
return fnTy->substGenericArgs(F.getModule(), M, subs);
699699
}
700700

701-
/// Check that for each opened archetype in substitutions, there is an
702-
/// opened archetype operand.
703-
void checkApplySubstitutionsOpenedArchetypes(SILInstruction *AI,
704-
ArrayRef<Substitution> Subs) {
705-
// If we have a substitution whose replacement type is an archetype, make
706-
// sure that the replacement archetype is in the context generic params of
707-
// the caller function.
701+
/// Check that for each opened archetype in substitutions
702+
/// or the calle type, there is a type dependent operand.
703+
void checkApplyTypeDependentArguments(ApplySite AS) {
704+
SILInstruction *AI = AS.getInstruction();
705+
708706
llvm::DenseSet<CanType> FoundOpenedArchetypes;
709-
for (auto &Sub : Subs) {
710-
Sub.getReplacement().visit([&](Type Ty) {
711-
if (!Ty->isOpenedExistential())
712-
return;
707+
708+
// Function to collect opened archetypes in FoundOpenedArchetypes.
709+
auto HandleType = [&](Type Ty) {
710+
if (Ty->isOpenedExistential()) {
713711
auto *A = Ty->getAs<ArchetypeType>();
714712
require(isArchetypeValidInFunction(A, AI->getFunction()),
715713
"Archetype to be substituted must be valid in function.");
@@ -718,35 +716,42 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
718716
// Also check that they are properly tracked inside the current
719717
// function.
720718
auto Def =
721-
OpenedArchetypes.getOpenedArchetypeDef(Ty.getCanonicalTypeOrNull());
719+
OpenedArchetypes.getOpenedArchetypeDef(Ty.getCanonicalTypeOrNull());
722720
require(Def, "Opened archetype should be registered in SILFunction");
723721
require(Def == AI ||
724-
Dominance->properlyDominates(cast<SILInstruction>(Def), AI),
722+
Dominance->properlyDominates(cast<SILInstruction>(Def), AI),
725723
"Use of an opened archetype should be dominated by a "
726724
"definition of this opened archetype");
727-
});
725+
}
726+
};
727+
728+
// Search for opened archetypes.
729+
for (auto &Sub : AS.getSubstitutions()) {
730+
Sub.getReplacement().visit(HandleType);
728731
}
732+
AS.getSubstCalleeType().visit(HandleType);
729733

730734
require(FoundOpenedArchetypes.size() ==
731735
AI->getOpenedArchetypeOperands().size(),
732-
"Number of opened archetypes in the substitutions list should "
733-
"match the number of opened archetype operands");
736+
"Number of opened archetypes in the substitutions "
737+
"list should match the number of type dependent operands");
734738

735739
for (auto &Op : AI->getOpenedArchetypeOperands()) {
736740
auto V = Op.get();
737741
require(isa<SILInstruction>(V),
738-
"opened archetype operand should refer to a SIL instruction");
742+
"opened archetype operand should refer to a SIL instruction");
739743
auto Archetype = getOpenedArchetypeOf(cast<SILInstruction>(V));
740-
require(Archetype, "opened archetype operand should define an opened archetype");
744+
require(Archetype,
745+
"opened archetype operand should define an opened archetype");
741746
require(FoundOpenedArchetypes.count(Archetype),
742-
"opened archetype operand does not correspond to any opened archetype from "
743-
"the substitutions list");
747+
"opened archetype operand does not correspond to any opened "
748+
"archetype from the substitutions list");
744749
}
745750
}
746751

747752
void checkFullApplySite(FullApplySite site) {
748-
checkApplySubstitutionsOpenedArchetypes(site.getInstruction(),
749-
site.getSubstitutions());
753+
checkApplyTypeDependentArguments(site);
754+
750755
// Then make sure that we have a type that can be substituted for the
751756
// callee.
752757
auto substTy = checkApplySubstitutions(site.getSubstitutions(),
@@ -866,7 +871,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
866871
require(resultInfo->getExtInfo().hasContext(),
867872
"result of closure cannot have a thin function type");
868873

869-
checkApplySubstitutionsOpenedArchetypes(PAI, PAI->getSubstitutions());
874+
checkApplyTypeDependentArguments(PAI);
870875

871876
auto substTy = checkApplySubstitutions(PAI->getSubstitutions(),
872877
PAI->getCallee()->getType());
@@ -1429,9 +1434,10 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
14291434
void checkMetatypeInst(MetatypeInst *MI) {
14301435
require(MI->getType().is<MetatypeType>(),
14311436
"metatype instruction must be of metatype type");
1432-
require(MI->getType().castTo<MetatypeType>()->hasRepresentation(),
1437+
auto MetaTy = MI->getType().castTo<MetatypeType>();
1438+
require(MetaTy->hasRepresentation(),
14331439
"metatype instruction must have a metatype representation");
1434-
verifyOpenedArchetype(MI, MI->getType().getSwiftRValueType());
1440+
verifyOpenedArchetype(MI, MetaTy.getInstanceType());
14351441
}
14361442
void checkValueMetatypeInst(ValueMetatypeInst *MI) {
14371443
require(MI->getType().is<MetatypeType>(),
@@ -1703,7 +1709,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
17031709
auto lookupType = AMI->getLookupType();
17041710
if (getOpenedArchetype(lookupType)) {
17051711
require(AMI->getOpenedArchetypeOperands().size() == 1,
1706-
"Must have an opened existential operand");
1712+
"Must have a type dependent operand for the opened archetype");
17071713
verifyOpenedArchetype(AMI, lookupType);
17081714
} else {
17091715
require(AMI->getOpenedArchetypeOperands().empty(),
@@ -1793,6 +1799,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
17931799
.getSwiftRValueType()
17941800
->isBindableTo(EMI->getType().getSwiftRValueType(), nullptr),
17951801
"result must be of the method's type");
1802+
verifyOpenedArchetype(EMI, EMI->getType().getSwiftRValueType());
17961803
}
17971804

17981805
void checkClassMethodInst(ClassMethodInst *CMI) {
@@ -2075,18 +2082,18 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
20752082
SILType resultType = I->getType();
20762083
require(resultType.is<ExistentialMetatypeType>(),
20772084
"init_existential_metatype result must be an existential metatype");
2085+
auto MetaTy = resultType.castTo<ExistentialMetatypeType>();
20782086
require(resultType.isObject(),
20792087
"init_existential_metatype result must not be an address");
2080-
require(resultType.castTo<ExistentialMetatypeType>()->hasRepresentation(),
2088+
require(MetaTy->hasRepresentation(),
20812089
"init_existential_metatype result must have a representation");
2082-
require(resultType.castTo<ExistentialMetatypeType>()->getRepresentation()
2090+
require(MetaTy->getRepresentation()
20832091
== operandType.castTo<MetatypeType>()->getRepresentation(),
20842092
"init_existential_metatype result must match representation of "
20852093
"operand");
20862094

20872095
checkExistentialProtocolConformances(resultType, I->getConformances());
2088-
verifyOpenedArchetype(
2089-
I, getOpenedArchetypeOf(I->getType().getSwiftRValueType()));
2096+
verifyOpenedArchetype(I, MetaTy.getInstanceType());
20902097
}
20912098

20922099
void checkExistentialProtocolConformances(SILType resultType,
@@ -2172,9 +2179,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
21722179
void verifyOpenedArchetype(SILInstruction *I, CanType Ty) {
21732180
if (!Ty)
21742181
return;
2175-
// Check for each referenced opened archetype from Ty
2176-
// that the instruction contains an opened archetype operand
2177-
// for it.
2182+
// Check the type and all of its contained types.
21782183
Ty.visit([&](Type t) {
21792184
if (!t->isOpenedExistential())
21802185
return;

‎lib/SILOptimizer/Transforms/CSE.cpp

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -681,13 +681,52 @@ bool CSE::processOpenExistentialRef(SILInstruction *Inst, ValueBase *V,
681681
auto &Builder = Cloner.getBuilder();
682682
Builder.setOpenedArchetypesTracker(&OpenedArchetypesTracker);
683683

684+
llvm::SmallPtrSet<SILInstruction *, 16> Processed;
684685
// Now clone each candidate and replace the opened archetype
685686
// by a dominating one.
686-
for (auto Candidate : Candidates) {
687+
while (!Candidates.empty()) {
688+
auto Candidate = Candidates.pop_back_val();
689+
if (Processed.count(Candidate))
690+
continue;
691+
// DependsOnOldOpenedArchetype is true if a candidate depends on the old
692+
// opened archetype.
693+
// First, check if it has any opened archetype operands.
694+
bool DependsOnOldOpenedArchetype =
695+
!Candidate->getOpenedArchetypeOperands().empty();
696+
// Then check if the result may depend on the opened archetype.
697+
if (!Candidate->use_empty() &&
698+
Candidate->getType().getSwiftRValueType()->hasOpenedExistential()) {
699+
// Check if the result type of the candidate depends on the opened
700+
// existential in question.
701+
auto ResultDependsOnOldOpenedArchetype =
702+
Candidate->getType().getSwiftRValueType().findIf(
703+
[&OldOpenedArchetype](Type t) -> bool {
704+
if (t.getCanonicalTypeOrNull() == OldOpenedArchetype)
705+
return true;
706+
return false;
707+
});
708+
if (ResultDependsOnOldOpenedArchetype) {
709+
DependsOnOldOpenedArchetype = true;
710+
// We need to update uses of this candidate, because their types
711+
// may be affected.
712+
for (auto Use : Candidate->getUses()) {
713+
Candidates.insert(Use->getUser());
714+
}
715+
}
716+
}
717+
// Remember that this candidate was processed already.
718+
Processed.insert(Candidate);
719+
720+
// No need to clone if there is no dependency on the old opened archetype.
721+
if (!DependsOnOldOpenedArchetype)
722+
continue;
723+
687724
Builder.getOpenedArchetypes().addOpenedArchetypeOperands(
688725
Candidate->getOpenedArchetypeOperands());
689726
Builder.setInsertionPoint(Candidate);
690727
auto NewI = Cloner.clone(Candidate);
728+
// Result types of candidate's uses instructions may be using this archetype.
729+
// Thus, we need to try to replace it there.
691730
Candidate->replaceAllUsesWith(NewI);
692731
if (I == Candidate->getIterator())
693732
I = NewI->getIterator();

‎test/SILOptimizer/cse.sil

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1325,6 +1325,8 @@ protocol Proto : class {
13251325
}
13261326

13271327
// Check that all open_existential_ref instructions are CSEd
1328+
// Any reference to the $@opened("1B685052-4796-11E6-B7DF-B8E856428C60") Proto should be replaced by
1329+
// references to $@opened("1B68354A-4796-11E6-B7DF-B8E856428C60") Proto.
13281330
// CHECK-LABEL: sil @cse_open_existential : $@convention(thin) (@guaranteed Proto, Bool) -> ()
13291331
// CHECK: bb0
13301332
// CHECK: %[[OPENED_EXISTENTIAL:[0-9]+]] = open_existential_ref %{{[0-9]+}} : $Proto to $@opened("1B68354A-4796-11E6-B7DF-B8E856428C60") Proto
@@ -1334,10 +1336,12 @@ protocol Proto : class {
13341336
// CHECK: bb1:
13351337
// CHECK-NEXT: %[[WM2:[0-9]+]] = witness_method $@opened("1B68354A-4796-11E6-B7DF-B8E856428C60") Proto, #Proto.doThat!1
13361338
// CHECK-NEXT: apply %[[WM2]]{{.*}}(%[[OPENED_EXISTENTIAL]])
1337-
// CHECK-NEXT: br bb
1339+
// CHECK-NOT: 1B6851A6-4796-11E6-B7DF-B8E856428C60
1340+
// CHECK: br bb
13381341
// CHECK: bb2:
13391342
// CHECK-NEXT: apply %[[WM1]]{{.*}}(%[[OPENED_EXISTENTIAL]])
1340-
// CHECK-NEXT: br bb
1343+
// CHECK-NOT: 1B6851A6-4796-11E6-B7DF-B8E856428C60
1344+
// CHECK: br bb
13411345
// CHECK: bb3:
13421346
// CHECK-NEXT: tuple
13431347
// CHECK-NEXT: return
@@ -1359,10 +1363,16 @@ bb2: // Preds: bb0
13591363
%13 = open_existential_ref %0 : $Proto to $@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto
13601364
%14 = witness_method $@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto, #Proto.doThis!1, %13 : $@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto : $@convention(witness_method) <τ_0_0 where τ_0_0 : Proto> (@guaranteed τ_0_0) -> ()
13611365
%15 = apply %14<@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto>(%13) : $@convention(witness_method) <τ_0_0 where τ_0_0 : Proto> (@guaranteed τ_0_0) -> ()
1366+
%16 = alloc_stack $@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto
1367+
store %13 to %16 : $*@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto
1368+
// This is to check that result types of instructions are updated by CSE as well.
1369+
%17 = load %16 : $*@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto
1370+
strong_release %17 : $@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto
1371+
dealloc_stack %16 : $*@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto
13621372
br bb3
13631373

13641374
bb3:
1365-
%17 = tuple ()
1366-
return %17 : $()
1375+
%20 = tuple ()
1376+
return %20 : $()
13671377
}
13681378

0 commit comments

Comments
 (0)