Merge pull request swiftlang#37036 from eeckstein/fix-open-archetype-mapping

SILModule: track opened archetypes per function.

Author: eeckstein

include/swift/SIL/SILFunction.h

@@ -1020,19 +1020,12 @@ class SILFunction
 
   /// Transfer all blocks of \p F into this function, at the begin of the block
   /// list.
-  void moveAllBlocksFromOtherFunction(SILFunction *F) {
-    BlockList.splice(begin(), F->BlockList);
-  }
+  void moveAllBlocksFromOtherFunction(SILFunction *F);
 
   /// Transfer \p blockInOtherFunction of another function into this function,
   /// before \p insertPointInThisFunction.
   void moveBlockFromOtherFunction(SILBasicBlock *blockInOtherFunction,
-                                  iterator insertPointInThisFunction) {
-    SILFunction *otherFunc = blockInOtherFunction->getParent();
-    assert(otherFunc != this);
-    BlockList.splice(insertPointInThisFunction, otherFunc->BlockList,
-                     blockInOtherFunction);
-  }
+                                  iterator insertPointInThisFunction);
 
   /// Move block \p BB to immediately before the iterator \p IP.
   ///

include/swift/SIL/SILModule.h

@@ -304,7 +304,13 @@ class SILModule {
   /// The value is either a SingleValueInstrution or a PlaceholderValue, in case
   /// an opened-archetype definition is lookedup during parsing or deserializing
   /// SIL, where opened archetypes can be forward referenced.
-  llvm::DenseMap<ArchetypeType*, SILValue> openedArchetypeDefs;
+  ///
+  /// In theory we wouldn't need to have the SILFunction in the key, because
+  /// opened archetypes _should_ be unique across the module. But currently
+  /// in some rare cases SILGen re-uses the same opened archetype for multiple
+  /// functions.
+  using OpenedArchetypeKey = std::pair<ArchetypeType*, SILFunction*>;
+  llvm::DenseMap<OpenedArchetypeKey, SILValue> openedArchetypeDefs;
 
   /// The number of PlaceholderValues in openedArchetypeDefs.
   int numUnresolvedOpenedArchetypes = 0;
@@ -382,15 +388,18 @@ class SILModule {
   /// In case the opened archetype is not defined yet (e.g. during parsing or
   /// deserilization), a PlaceholderValue is returned. This should not be the
   /// case outside of parsing or deserialization.
-  SILValue getOpenedArchetypeDef(CanArchetypeType archetype);
+  SILValue getOpenedArchetypeDef(CanArchetypeType archetype,
+                                 SILFunction *inFunction);
 
   /// Returns the instruction which defines an opened archetype, e.g. an
   /// open_existential_addr.
   ///
   /// In contrast to getOpenedArchetypeDef, it is required that all opened
   /// archetypes are resolved.
-  SingleValueInstruction *getOpenedArchetypeInst(CanArchetypeType archetype) {
-    return cast<SingleValueInstruction>(getOpenedArchetypeDef(archetype));
+  SingleValueInstruction *getOpenedArchetypeInst(CanArchetypeType archetype,
+                                                 SILFunction *inFunction) {
+    return cast<SingleValueInstruction>(getOpenedArchetypeDef(archetype,
+                                                              inFunction));
   }
 
   /// Returns true if there are unresolved opened archetypes in the module.
@@ -401,6 +410,9 @@ class SILModule {
   /// Called by SILBuilder whenever a new instruction is created and inserted.
   void notifyAddedInstruction(SILInstruction *inst);
 
+  /// Called after an instruction is moved from one function to another.
+  void notifyMovedInstruction(SILInstruction *inst, SILFunction *fromFunction);
+
   /// Add a delete notification handler \p Handler to the module context.
   void registerDeleteNotificationHandler(DeleteNotificationHandler* Handler);
 

lib/SIL/IR/SILFunction.cpp

@@ -383,6 +383,30 @@ SILBasicBlock *SILFunction::createBasicBlockBefore(SILBasicBlock *beforeBB) {
   return newBlock;
 }
 
+void SILFunction::moveAllBlocksFromOtherFunction(SILFunction *F) {
+  BlockList.splice(begin(), F->BlockList);
+  
+  SILModule &mod = getModule();
+  for (SILBasicBlock &block : *this) {
+    for (SILInstruction &inst : block) {
+      mod.notifyMovedInstruction(&inst, F);
+    }
+  }
+}
+
+void SILFunction::moveBlockFromOtherFunction(SILBasicBlock *blockInOtherFunction,
+                                iterator insertPointInThisFunction) {
+  SILFunction *otherFunc = blockInOtherFunction->getParent();
+  assert(otherFunc != this);
+  BlockList.splice(insertPointInThisFunction, otherFunc->BlockList,
+                   blockInOtherFunction);
+
+  SILModule &mod = getModule();
+  for (SILInstruction &inst : *blockInOtherFunction) {
+    mod.notifyMovedInstruction(&inst, otherFunc);
+  }
+}
+
 void SILFunction::moveBlockBefore(SILBasicBlock *BB, SILFunction::iterator IP) {
   assert(BB->getParent() == this);
   if (SILFunction::iterator(BB) == IP)

lib/SIL/IR/SILInstructions.cpp

@@ -76,7 +76,7 @@ static void buildTypeDependentOperands(
     SmallVectorImpl<SILValue> &TypeDependentOperands, SILFunction &F) {
 
   for (auto archetype : OpenedArchetypes) {
-    SILValue def = F.getModule().getOpenedArchetypeDef(archetype);
+    SILValue def = F.getModule().getOpenedArchetypeDef(archetype, &F);
     assert(def->getFunction() == &F &&
            "def of opened archetype is in wrong function");
     TypeDependentOperands.push_back(def);

lib/SIL/IR/SILModule.cpp

@@ -724,8 +724,9 @@ void SILModule::registerDeserializationNotificationHandler(
   deserializationNotificationHandlers.add(std::move(handler));
 }
 
-SILValue SILModule::getOpenedArchetypeDef(CanArchetypeType archetype) {
-  SILValue &def = openedArchetypeDefs[archetype];
+SILValue SILModule::getOpenedArchetypeDef(CanArchetypeType archetype,
+                                          SILFunction *inFunction) {
+  SILValue &def = openedArchetypeDefs[{archetype, inFunction}];
   if (!def) {
     numUnresolvedOpenedArchetypes++;
     def = ::new PlaceholderValue(SILType::getPrimitiveAddressType(archetype));
@@ -741,7 +742,7 @@ bool SILModule::hasUnresolvedOpenedArchetypeDefinitions() {
 void SILModule::notifyAddedInstruction(SILInstruction *inst) {
   if (auto *svi = dyn_cast<SingleValueInstruction>(inst)) {
     if (CanArchetypeType archeTy = svi->getOpenedArchetype()) {
-      SILValue &val = openedArchetypeDefs[archeTy];
+      SILValue &val = openedArchetypeDefs[{archeTy, inst->getFunction()}];
       if (val) {
         if (!isa<PlaceholderValue>(val)) {
           // Print a useful error message (and not just abort with an assert).
@@ -765,6 +766,19 @@ void SILModule::notifyAddedInstruction(SILInstruction *inst) {
   }
 }
 
+void SILModule::notifyMovedInstruction(SILInstruction *inst,
+                                       SILFunction *fromFunction) {
+  if (auto *svi = dyn_cast<SingleValueInstruction>(inst)) {
+    if (CanArchetypeType archeTy = svi->getOpenedArchetype()) {
+      OpenedArchetypeKey key = {archeTy, fromFunction};
+      assert(openedArchetypeDefs.lookup(key) == svi &&
+             "archetype def was not registered");
+      openedArchetypeDefs.erase(key);
+      openedArchetypeDefs[{archeTy, svi->getFunction()}] = svi;
+    }
+  }
+}
+
 void SILModule::registerDeleteNotificationHandler(
     DeleteNotificationHandler *handler) {
   // Ask the handler (that can be an analysis, a pass, or some other data
@@ -783,9 +797,10 @@ void SILModule::notifyDeleteHandlers(SILNode *node) {
   // Update openedArchetypeDefs.
   if (auto *svi = dyn_cast<SingleValueInstruction>(node)) {
     if (CanArchetypeType archeTy = svi->getOpenedArchetype()) {
-      assert(openedArchetypeDefs.lookup(archeTy) == svi &&
+      OpenedArchetypeKey key = {archeTy, svi->getFunction()};
+      assert(openedArchetypeDefs.lookup(key) == svi &&
              "archetype def was not registered");
-      openedArchetypeDefs.erase(archeTy);
+      openedArchetypeDefs.erase(key);
     }
   }
 

lib/SIL/Verifier/SILVerifier.cpp

@@ -1322,7 +1322,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
               "Operand is of an ArchetypeType that does not exist in the "
               "Caller's generic param list.");
       if (auto OpenedA = getOpenedArchetypeOf(A)) {
-        auto *openingInst = F->getModule().getOpenedArchetypeInst(OpenedA);
+        auto *openingInst = F->getModule().getOpenedArchetypeInst(OpenedA, F);
         require(I == nullptr || openingInst == I ||
                 properlyDominates(openingInst, I),
                 "Use of an opened archetype should be dominated by a "
@@ -1456,7 +1456,8 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
         FoundOpenedArchetypes.insert(A);
         // Also check that they are properly tracked inside the current
         // function.
-        auto *openingInst = F.getModule().getOpenedArchetypeInst(A);
+        auto *openingInst = F.getModule().getOpenedArchetypeInst(A,
+                              AI->getFunction());
         require(openingInst == AI ||
                 properlyDominates(openingInst, AI),
                 "Use of an opened archetype should be dominated by a "
@@ -3404,7 +3405,8 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     auto archetype = getOpenedArchetypeOf(OEI->getType().getASTType());
     require(archetype,
         "open_existential_addr result must be an opened existential archetype");
-    require(OEI->getModule().getOpenedArchetypeInst(archetype) == OEI,
+    require(OEI->getModule().getOpenedArchetypeInst(archetype,
+                 OEI->getFunction()) == OEI,
             "Archetype opened by open_existential_addr should be registered in "
             "SILFunction");
 
@@ -3437,7 +3439,8 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     auto archetype = getOpenedArchetypeOf(resultInstanceTy);
     require(archetype,
         "open_existential_ref result must be an opened existential archetype");
-    require(OEI->getModule().getOpenedArchetypeInst(archetype) == OEI,
+    require(OEI->getModule().getOpenedArchetypeInst(archetype,
+                 OEI->getFunction()) == OEI,
             "Archetype opened by open_existential_ref should be registered in "
             "SILFunction");
   }
@@ -3459,7 +3462,8 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     auto archetype = getOpenedArchetypeOf(resultInstanceTy);
     require(archetype,
         "open_existential_box result must be an opened existential archetype");
-    require(OEI->getModule().getOpenedArchetypeInst(archetype) == OEI,
+    require(OEI->getModule().getOpenedArchetypeInst(archetype,
+                 OEI->getFunction()) == OEI,
             "Archetype opened by open_existential_box should be registered in "
             "SILFunction");
   }
@@ -3481,7 +3485,8 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     auto archetype = getOpenedArchetypeOf(resultInstanceTy);
     require(archetype,
         "open_existential_box_value result not an opened existential archetype");
-    require(OEI->getModule().getOpenedArchetypeInst(archetype) == OEI,
+    require(OEI->getModule().getOpenedArchetypeInst(archetype,
+                 OEI->getFunction()) == OEI,
             "Archetype opened by open_existential_box_value should be "
             "registered in SILFunction");
   }
@@ -3527,7 +3532,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     require(archetype, "open_existential_metatype result must be an opened "
                        "existential metatype");
     require(
-        I->getModule().getOpenedArchetypeInst(archetype) == I,
+        I->getModule().getOpenedArchetypeInst(archetype, I->getFunction()) == I,
         "Archetype opened by open_existential_metatype should be registered in "
         "SILFunction");
   }
@@ -3546,7 +3551,8 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     auto archetype = getOpenedArchetypeOf(OEI->getType().getASTType());
     require(archetype, "open_existential_value result must be an opened "
                        "existential archetype");
-    require(OEI->getModule().getOpenedArchetypeInst(archetype) == OEI,
+    require(OEI->getModule().getOpenedArchetypeInst(archetype,
+                 OEI->getFunction()) == OEI,
             "Archetype opened by open_existential should be registered in "
             "SILFunction");
   }
@@ -3834,7 +3840,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
       SILValue Def;
       if (t->isOpenedExistential()) {
         auto archetypeTy = cast<ArchetypeType>(t);
-        Def = I->getModule().getOpenedArchetypeInst(archetypeTy);
+        Def = I->getModule().getOpenedArchetypeInst(archetypeTy, I->getFunction());
         require(Def, "Opened archetype should be registered in SILModule");
       } else if (t->hasDynamicSelfType()) {
         require(I->getFunction()->hasSelfParam() ||

test/SILGen/Inputs/duplicate_opened_archetypes.h

@@ -0,0 +1,7 @@
+
+@protocol P
+@end
+
+@interface I
+@property(class, readonly) I<P> *x;
+@end

test/SILGen/duplicate_opened_archetypes.swift

@@ -0,0 +1,33 @@
+// RUN: %target-swift-frontend %s -enable-objc-interop -import-objc-header %S/Inputs/duplicate_opened_archetypes.h -emit-sil -o /dev/null
+
+// REQUIRES: objc_interop
+
+// Check that SILGen does not crash because of duplicate opened archetypes
+// in two functions.
+
+import Foundation
+
+struct S {
+    let i: I
+}
+
+@propertyWrapper
+public struct W<Value> {
+    public var wrappedValue: Value
+
+    public init(wrappedValue: Value) {
+        self.wrappedValue = wrappedValue
+    }
+
+    public init(initialValue: Value) {
+        wrappedValue = initialValue
+    }
+
+}
+
+
+struct Test {
+  @W private var s = S(i: .x)
+
+  var t: S = S(i: .x)
+}