[sil-cse] Fix a bug in the CSE of open_existential_ref instructions

When performing a CSE of open_existential_ref instructions, we replace the new archetype by the old archetype by cloning the uses and re-mapping the archetypes. But we also need to consider that some of the uses of a open_existential_ref instruction (e.g. loads) may produce results depending on the opened archetype being replaced. Therefore, for every such use its own uses (and their uses) should be eventually recursively cloned and type-remapped as well if they depend on the opened archetype being replaced.

Fixes rdar://28136015 and https://bugs.swift.org/browse/SR-2545

Author: swiftix

include/swift/SIL/SILBuilder.h

@@ -954,8 +954,9 @@ class SILBuilder {
   DynamicMethodInst *createDynamicMethod(SILLocation Loc, SILValue Operand,
                                          SILDeclRef Member, SILType MethodTy,
                                          bool Volatile = false) {
-    return insert(new (F.getModule()) DynamicMethodInst(
-        getSILDebugLocation(Loc), Operand, Member, MethodTy, Volatile));
+    return insert(DynamicMethodInst::create(
+        getSILDebugLocation(Loc), Operand, Member, MethodTy, Volatile,
+        &F, OpenedArchetypes));
   }
 
   OpenExistentialAddrInst *

include/swift/SIL/SILCloner.h

@@ -270,6 +270,7 @@ class SILBuilderWithPostProcess : public SILBuilder {
     {
       setInsertionPoint(SC.getBuilder().getInsertionBB(),
                         SC.getBuilder().getInsertionPoint());
+      setOpenedArchetypesTracker(SC.getBuilder().getOpenedArchetypesTracker());
     }
 
   ~SILBuilderWithPostProcess() {

include/swift/SIL/SILInstruction.h

@@ -3613,14 +3613,25 @@ class WitnessMethodInst final
 /// constant referring to some Objective-C method, performs dynamic method
 /// lookup to extract the implementation of that method. This method lookup
 /// can fail at run-time
-class DynamicMethodInst
-  : public UnaryInstructionBase<ValueKind::DynamicMethodInst, MethodInst>
+class DynamicMethodInst final
+  : public UnaryInstructionWithOpenArchetypesBase<
+                                   ValueKind::DynamicMethodInst,
+                                   DynamicMethodInst,
+                                   MethodInst,
+                                   true>
 {
   friend class SILBuilder;
 
   DynamicMethodInst(SILDebugLocation DebugLoc, SILValue Operand,
-                    SILDeclRef Member, SILType Ty, bool Volatile = false)
-      : UnaryInstructionBase(DebugLoc, Operand, Ty, Member, Volatile) {}
+                    ArrayRef<SILValue> TypeDependentOperands,
+                    SILDeclRef Member, SILType Ty, bool Volatile)
+      : UnaryInstructionWithOpenArchetypesBase(DebugLoc, Operand,
+                               TypeDependentOperands, Ty, Member, Volatile) {}
+
+  static DynamicMethodInst *
+  create(SILDebugLocation DebugLoc, SILValue Operand,
+         SILDeclRef Member, SILType Ty, bool Volatile, SILFunction *F,
+         SILOpenedArchetypesState &OpenedArchetypes);
 };
 
 /// Given the address of an existential, "opens" the

lib/SIL/SILInstructions.cpp

@@ -698,18 +698,16 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     return fnTy->substGenericArgs(F.getModule(), M, subs);
   }
 
-  /// Check that for each opened archetype in substitutions, there is an
-  /// opened archetype operand.
-  void checkApplySubstitutionsOpenedArchetypes(SILInstruction *AI,
-                                               ArrayRef<Substitution> Subs) {
-    // If we have a substitution whose replacement type is an archetype, make
-    // sure that the replacement archetype is in the context generic params of
-    // the caller function.
+  /// Check that for each opened archetype in substitutions
+  /// or the calle type, there is a type dependent operand.
+  void checkApplyTypeDependentArguments(ApplySite AS) {
+    SILInstruction *AI = AS.getInstruction();
+
     llvm::DenseSet<CanType> FoundOpenedArchetypes;
-    for (auto &Sub : Subs) {
-      Sub.getReplacement().visit([&](Type Ty) {
-        if (!Ty->isOpenedExistential())
-          return;
+
+    // Function to collect opened archetypes in FoundOpenedArchetypes.
+    auto HandleType = [&](Type Ty) {
+      if (Ty->isOpenedExistential()) {
         auto *A = Ty->getAs<ArchetypeType>();
         require(isArchetypeValidInFunction(A, AI->getFunction()),
                 "Archetype to be substituted must be valid in function.");
@@ -718,35 +716,42 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
         // Also check that they are properly tracked inside the current
         // function.
         auto Def =
-            OpenedArchetypes.getOpenedArchetypeDef(Ty.getCanonicalTypeOrNull());
+          OpenedArchetypes.getOpenedArchetypeDef(Ty.getCanonicalTypeOrNull());
         require(Def, "Opened archetype should be registered in SILFunction");
         require(Def == AI ||
-                    Dominance->properlyDominates(cast<SILInstruction>(Def), AI),
+                Dominance->properlyDominates(cast<SILInstruction>(Def), AI),
                 "Use of an opened archetype should be dominated by a "
                 "definition of this opened archetype");
-      });
+      }
+    };
+
+    // Search for opened archetypes.
+    for (auto &Sub : AS.getSubstitutions()) {
+      Sub.getReplacement().visit(HandleType);
     }
+    AS.getSubstCalleeType().visit(HandleType);
 
     require(FoundOpenedArchetypes.size() ==
                 AI->getOpenedArchetypeOperands().size(),
-            "Number of opened archetypes in the substitutions list should "
-            "match the number of opened archetype operands");
+            "Number of opened archetypes in the substitutions "
+            "list should match the number of type dependent operands");
 
     for (auto &Op : AI->getOpenedArchetypeOperands()) {
       auto V = Op.get();
       require(isa<SILInstruction>(V),
-             "opened archetype operand should refer to a SIL instruction");
+              "opened archetype operand should refer to a SIL instruction");
       auto Archetype = getOpenedArchetypeOf(cast<SILInstruction>(V));
-      require(Archetype, "opened archetype operand should define an opened archetype");
+      require(Archetype,
+              "opened archetype operand should define an opened archetype");
       require(FoundOpenedArchetypes.count(Archetype),
-              "opened archetype operand does not correspond to any opened archetype from "
-              "the substitutions list");
+              "opened archetype operand does not correspond to any opened "
+              "archetype from the substitutions list");
     }
   }
 
   void checkFullApplySite(FullApplySite site) {
-    checkApplySubstitutionsOpenedArchetypes(site.getInstruction(),
-                                            site.getSubstitutions());
+    checkApplyTypeDependentArguments(site);
+
     // Then make sure that we have a type that can be substituted for the
     // callee.
     auto substTy = checkApplySubstitutions(site.getSubstitutions(),
@@ -866,7 +871,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     require(resultInfo->getExtInfo().hasContext(),
             "result of closure cannot have a thin function type");
 
-    checkApplySubstitutionsOpenedArchetypes(PAI, PAI->getSubstitutions());
+    checkApplyTypeDependentArguments(PAI);
 
     auto substTy = checkApplySubstitutions(PAI->getSubstitutions(),
                                         PAI->getCallee()->getType());
@@ -1429,9 +1434,10 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
   void checkMetatypeInst(MetatypeInst *MI) {
     require(MI->getType().is<MetatypeType>(),
             "metatype instruction must be of metatype type");
-    require(MI->getType().castTo<MetatypeType>()->hasRepresentation(),
+    auto MetaTy = MI->getType().castTo<MetatypeType>();
+    require(MetaTy->hasRepresentation(),
             "metatype instruction must have a metatype representation");
-    verifyOpenedArchetype(MI, MI->getType().getSwiftRValueType());
+    verifyOpenedArchetype(MI, MetaTy.getInstanceType());
   }
   void checkValueMetatypeInst(ValueMetatypeInst *MI) {
     require(MI->getType().is<MetatypeType>(),
@@ -1703,7 +1709,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     auto lookupType = AMI->getLookupType();
     if (getOpenedArchetype(lookupType)) {
       require(AMI->getOpenedArchetypeOperands().size() == 1,
-              "Must have an opened existential operand");
+              "Must have a type dependent operand for the opened archetype");
       verifyOpenedArchetype(AMI, lookupType);
     } else {
       require(AMI->getOpenedArchetypeOperands().empty(),
@@ -1793,6 +1799,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
               .getSwiftRValueType()
               ->isBindableTo(EMI->getType().getSwiftRValueType(), nullptr),
             "result must be of the method's type");
+    verifyOpenedArchetype(EMI, EMI->getType().getSwiftRValueType());
   }
 
   void checkClassMethodInst(ClassMethodInst *CMI) {
@@ -2075,18 +2082,18 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     SILType resultType = I->getType();
     require(resultType.is<ExistentialMetatypeType>(),
             "init_existential_metatype result must be an existential metatype");
+    auto MetaTy = resultType.castTo<ExistentialMetatypeType>();
     require(resultType.isObject(),
             "init_existential_metatype result must not be an address");
-    require(resultType.castTo<ExistentialMetatypeType>()->hasRepresentation(),
+    require(MetaTy->hasRepresentation(),
             "init_existential_metatype result must have a representation");
-    require(resultType.castTo<ExistentialMetatypeType>()->getRepresentation()
+    require(MetaTy->getRepresentation()
               == operandType.castTo<MetatypeType>()->getRepresentation(),
             "init_existential_metatype result must match representation of "
             "operand");
 
     checkExistentialProtocolConformances(resultType, I->getConformances());
-    verifyOpenedArchetype(
-        I, getOpenedArchetypeOf(I->getType().getSwiftRValueType()));
+    verifyOpenedArchetype(I, MetaTy.getInstanceType());
   }
 
   void checkExistentialProtocolConformances(SILType resultType,
@@ -2172,9 +2179,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
   void verifyOpenedArchetype(SILInstruction *I, CanType Ty) {
     if (!Ty)
       return;
-    // Check for each referenced opened archetype from Ty
-    // that the instruction contains an opened archetype operand
-    // for it.
+    // Check the type and all of its contained types.
     Ty.visit([&](Type t) {
       if (!t->isOpenedExistential())
         return;

lib/SIL/SILVerifier.cpp

@@ -681,13 +681,52 @@ bool CSE::processOpenExistentialRef(SILInstruction *Inst, ValueBase *V,
   auto &Builder = Cloner.getBuilder();
   Builder.setOpenedArchetypesTracker(&OpenedArchetypesTracker);
 
+  llvm::SmallPtrSet<SILInstruction *, 16> Processed;
   // Now clone each candidate and replace the opened archetype
   // by a dominating one.
-  for (auto Candidate : Candidates) {
+  while (!Candidates.empty()) {
+    auto Candidate = Candidates.pop_back_val();
+    if (Processed.count(Candidate))
+      continue;
+    // DependsOnOldOpenedArchetype is true if a candidate depends on the old
+    // opened archetype.
+    // First, check if it has any opened archetype operands.
+    bool DependsOnOldOpenedArchetype =
+        !Candidate->getOpenedArchetypeOperands().empty();
+    // Then check if the result may depend on the opened archetype.
+    if (!Candidate->use_empty() &&
+        Candidate->getType().getSwiftRValueType()->hasOpenedExistential()) {
+      // Check if the result type of the candidate depends on the opened
+      // existential in question.
+      auto ResultDependsOnOldOpenedArchetype =
+          Candidate->getType().getSwiftRValueType().findIf(
+              [&OldOpenedArchetype](Type t) -> bool {
+                if (t.getCanonicalTypeOrNull() == OldOpenedArchetype)
+                  return true;
+                return false;
+              });
+      if (ResultDependsOnOldOpenedArchetype) {
+        DependsOnOldOpenedArchetype = true;
+        // We need to update uses of this candidate, because their types
+        // may be affected.
+        for (auto Use : Candidate->getUses()) {
+          Candidates.insert(Use->getUser());
+        }
+      }
+    }
+    // Remember that this candidate was processed already.
+    Processed.insert(Candidate);
+
+    // No need to clone if there is no dependency on the old opened archetype.
+    if (!DependsOnOldOpenedArchetype)
+      continue;
+
     Builder.getOpenedArchetypes().addOpenedArchetypeOperands(
         Candidate->getOpenedArchetypeOperands());
     Builder.setInsertionPoint(Candidate);
     auto NewI = Cloner.clone(Candidate);
+    // Result types of candidate's uses instructions may be using this archetype.
+    // Thus, we need to try to replace it there.
     Candidate->replaceAllUsesWith(NewI);
     if (I == Candidate->getIterator())
       I = NewI->getIterator();

lib/SILOptimizer/Transforms/CSE.cpp

@@ -1325,6 +1325,8 @@ protocol Proto : class {
 }
 
 // Check that all open_existential_ref instructions are CSEd 
+// Any reference to the $@opened("1B685052-4796-11E6-B7DF-B8E856428C60") Proto should be replaced by
+// references to $@opened("1B68354A-4796-11E6-B7DF-B8E856428C60") Proto.
 // CHECK-LABEL: sil @cse_open_existential : $@convention(thin) (@guaranteed Proto, Bool) -> ()
 // CHECK: bb0
 // 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 {
 // CHECK: bb1:
 // CHECK-NEXT: %[[WM2:[0-9]+]] = witness_method $@opened("1B68354A-4796-11E6-B7DF-B8E856428C60") Proto, #Proto.doThat!1
 // CHECK-NEXT: apply %[[WM2]]{{.*}}(%[[OPENED_EXISTENTIAL]])
-// CHECK-NEXT: br bb
+// CHECK-NOT: 1B6851A6-4796-11E6-B7DF-B8E856428C60
+// CHECK: br bb
 // CHECK: bb2:
 // CHECK-NEXT: apply %[[WM1]]{{.*}}(%[[OPENED_EXISTENTIAL]])
-// CHECK-NEXT: br bb
+// CHECK-NOT: 1B6851A6-4796-11E6-B7DF-B8E856428C60
+// CHECK: br bb
 // CHECK: bb3:
 // CHECK-NEXT: tuple
 // CHECK-NEXT: return
@@ -1359,10 +1363,16 @@ bb2:                                              // Preds: bb0
   %13 = open_existential_ref %0 : $Proto to $@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto
   %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) -> ()
   %15 = apply %14<@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto>(%13) : $@convention(witness_method) <τ_0_0 where τ_0_0 : Proto> (@guaranteed τ_0_0) -> ()
+  %16 = alloc_stack $@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto
+  store %13 to %16 : $*@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto
+  // This is to check that result types of instructions are updated by CSE as well.
+  %17 = load %16 : $*@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto
+  strong_release %17 : $@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto
+  dealloc_stack %16 : $*@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto
   br bb3
 
 bb3:
-  %17 = tuple ()
-  return %17 : $()
+  %20 = tuple ()
+  return %20 : $()
 }