SILGen: Open existential lvalues on entry into an OpenExistentialExpr again.

4b25945 changed codegen for lvalue OpenExistentialExprs so that the existential was not opened until the OpaqueValue's lvalue was evaluated, but this is incorrect—we need to open the dynamic type of the existential immediately since it can be used arbitrarily within the subexpression. This caused a regression when evaluating default argument generators on protocol extension methods (rdar://problem/37031037), and would become a bigger problem when we generalize the ability to open existentials.

Author: jckarter

lib/SILGen/SILGenExpr.cpp

@@ -5091,15 +5091,34 @@ RValue RValueEmitter::emitForceValue(ForceValueExpr *loc, Expr *E,
 void SILGenFunction::emitOpenExistentialExprImpl(
        OpenExistentialExpr *E,
        llvm::function_ref<void(Expr *)> emitSubExpr) {
-  Optional<FormalEvaluationScope> writebackScope;
-
   // Emit the existential value.
   if (E->getExistentialValue()->getType()->is<LValueType>()) {
-    bool inserted = OpaqueValueExprs.insert({E->getOpaqueValue(), E}).second;
+    // Open the existential container right away. We need the dynamic type
+    // to be opened in order to evaluate the subexpression.
+    AccessKind accessKind;
+    if (E->hasLValueAccessKind())
+      accessKind = E->getLValueAccessKind();
+    else
+      accessKind = E->getExistentialValue()->getLValueAccessKind();
+    auto lv = emitLValue(E->getExistentialValue(), accessKind);
+    auto formalOpenedType = E->getOpaqueValue()->getType()
+                                    ->getWithoutSpecifierType()
+                                    ->getCanonicalType();
+    lv = emitOpenExistentialLValue(E, std::move(lv),
+                                   CanArchetypeType(E->getOpenedArchetype()),
+                                   formalOpenedType,
+                                   accessKind);
+    
+    auto addr = emitAddressOfLValue(E, std::move(lv), accessKind);
+    bool inserted = OpaqueLValues.insert({E->getOpaqueValue(),
+                                          {addr.getValue(), formalOpenedType}})
+      .second;
     (void)inserted;
     assert(inserted && "already have this opened existential?");
 
     emitSubExpr(E->getSubExpr());
+    assert(OpaqueLValues.count(E->getOpaqueValue()) == 0
+           && "opened existential not removed?");
     return;
   }
 
@@ -5126,6 +5145,12 @@ RValue RValueEmitter::visitOpenExistentialExpr(OpenExistentialExpr *E,
     return RValue(SGF, E, *result);
   }
 
+  Optional<FormalEvaluationScope> scope;
+  // Begin an evaluation scope for an lvalue existential opened into an
+  // rvalue expression.
+  if (E->getExistentialValue()->getType()->is<LValueType>())
+    scope.emplace(SGF);
+  
   return SGF.emitOpenExistentialExpr<RValue>(E,
                                              [&](Expr *subExpr) -> RValue {
                                                return visit(subExpr, C);

lib/SILGen/SILGenFunction.h

@@ -1417,10 +1417,10 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
   llvm::SmallDenseMap<OpaqueValueExpr *, OpaqueValueState>
     OpaqueValues;
 
-  /// A mapping from opaque value expressions to the open-existential
-  /// expression that determines them, used while lowering lvalues.
-  llvm::SmallDenseMap<OpaqueValueExpr *, OpenExistentialExpr *>
-    OpaqueValueExprs;
+  /// A mapping from opaque value lvalue expressions to the address of the
+  /// opened value in memory.
+  llvm::SmallDenseMap<OpaqueValueExpr *, std::pair<SILValue, CanType>>
+    OpaqueLValues;
 
   /// RAII object that introduces a temporary binding for an opaque value.
   ///

lib/SILGen/SILGenLValue.cpp

@@ -2208,20 +2208,18 @@ LValue SILGenLValue::visitOpaqueValueExpr(OpaqueValueExpr *e,
                                           AccessKind accessKind,
                                           LValueOptions options) {
   // Handle an opaque lvalue that refers to an opened existential.
-  auto known = SGF.OpaqueValueExprs.find(e);
-  if (known != SGF.OpaqueValueExprs.end()) {
-    // Dig the open-existential expression out of the list.
-    OpenExistentialExpr *opened = known->second;
-    SGF.OpaqueValueExprs.erase(known);
-
-    // Do formal evaluation of the underlying existential lvalue.
-    auto lv = visitRec(opened->getExistentialValue(), accessKind, options);
-    lv = SGF.emitOpenExistentialLValue(
-        opened, std::move(lv),
-        CanArchetypeType(opened->getOpenedArchetype()),
-        e->getType()->getWithoutSpecifierType()->getCanonicalType(),
-        accessKind);
-    return lv;
+  auto known = SGF.OpaqueLValues.find(e);
+  if (known != SGF.OpaqueLValues.end()) {
+    // Dig the opened address out of the list.
+    SILValue opened = known->second.first;
+    CanType openedType = known->second.second;
+    SGF.OpaqueLValues.erase(known);
+
+    // Continue formal evaluation of the lvalue from this address.
+    return LValue::forAddress(ManagedValue::forLValue(opened),
+                              None,
+                              AbstractionPattern(openedType),
+                              openedType);
   }
 
   assert(SGF.OpaqueValues.count(e) && "Didn't bind OpaqueValueExpr");
@@ -2513,29 +2511,12 @@ LValue SILGenLValue::visitTupleElementExpr(TupleElementExpr *e,
 LValue SILGenLValue::visitOpenExistentialExpr(OpenExistentialExpr *e,
                                               AccessKind accessKind,
                                               LValueOptions options) {
-  // If the opaque value is not an lvalue, open the existential immediately.
-  if (!e->getOpaqueValue()->getType()->is<LValueType>()) {
-    return SGF.emitOpenExistentialExpr<LValue>(e,
-                                               [&](Expr *subExpr) -> LValue {
-                                                 return visitRec(subExpr,
-                                                                 accessKind,
-                                                                 options);
-                                               });
-  }
-
-  // Record the fact that we're opening this existential. The actual
-  // opening operation will occur when we see the OpaqueValueExpr.
-  bool inserted = SGF.OpaqueValueExprs.insert({e->getOpaqueValue(), e}).second;
-  (void)inserted;
-  assert(inserted && "already have this opened existential?");
-
-  // Visit the subexpression.
-  LValue lv = visitRec(e->getSubExpr(), accessKind, options);
-
-  // Sanity check that we did see the OpaqueValueExpr.
-  assert(SGF.OpaqueValueExprs.count(e->getOpaqueValue()) == 0 &&
-         "opened existential not removed?");
-  return lv;
+  return SGF.emitOpenExistentialExpr<LValue>(e,
+                                             [&](Expr *subExpr) -> LValue {
+                                               return visitRec(subExpr,
+                                                               accessKind,
+                                                               options);
+                                             });
 }
 
 static LValueTypeData

test/SILGen/assignment.swift

@@ -43,11 +43,11 @@ protocol P {
 // CHECK-LABEL: sil hidden @$S10assignment15copyRightToLeft1pyAA1P_pz_tF : $@convention(thin) (@inout P) -> () {
 func copyRightToLeft(p: inout P) {
   // CHECK: bb0(%0 : @trivial $*P):
+  // CHECK:   [[WRITE:%.*]] = begin_access [modify] [unknown] %0 : $*P
+  // CHECK:   [[WRITE_OPEN:%.*]] = open_existential_addr mutable_access [[WRITE]]
   // CHECK:   [[READ:%.*]] = begin_access [read] [unknown] %0 : $*P
   // CHECK:   [[READ_OPEN:%.*]] = open_existential_addr immutable_access [[READ]]
   // CHECK:   end_access [[READ]] : $*P
-  // CHECK:   [[WRITE:%.*]] = begin_access [modify] [unknown] %0 : $*P
-  // CHECK:   [[WRITE_OPEN:%.*]] = open_existential_addr mutable_access [[WRITE]]
   // CHECK:   end_access [[WRITE]] : $*P
   p.left = p.right
 }

test/SILGen/class_bound_protocols.swift

@@ -192,22 +192,23 @@ func takesInheritsMutatingMethod(x: inout InheritsMutatingMethod,
   // CHECK-NEXT: [[X_PAYLOAD:%.*]] = open_existential_ref [[X_VALUE]] : $InheritsMutatingMethod to $@opened("{{.*}}") InheritsMutatingMethod
   // CHECK-NEXT: [[TEMPORARY:%.*]] = alloc_stack $@opened("{{.*}}") InheritsMutatingMethod
   // CHECK-NEXT: store [[X_PAYLOAD]] to [init] [[TEMPORARY]] : $*@opened("{{.*}}") InheritsMutatingMethod
-  // CHECK-NEXT: [[X_PAYLOAD_RELOADED:%.*]] = load [take] [[TEMPORARY]]
+  // CHECK-NEXT: [[X_PAYLOAD_RELOADED:%.*]] = load_borrow [[TEMPORARY]]
   //
   // ** *NOTE* This extra copy is here since RValue invariants enforce that all
   // ** loadable objects are actually loaded. So we form the RValue and
   // ** load... only to then need to store the value back in a stack location to
   // ** pass to an in_guaranteed method. PredictableMemOpts is able to handle this
   // ** type of temporary codegen successfully.
   // CHECK-NEXT: [[TEMPORARY_2:%.*]] = alloc_stack $@opened("{{.*}}") InheritsMutatingMethod
-  // CHECK-NEXT: store [[X_PAYLOAD_RELOADED:%.*]] to [init] [[TEMPORARY_2]]
+  // CHECK-NEXT: store_borrow [[X_PAYLOAD_RELOADED:%.*]] to [[TEMPORARY_2]]
   // 
   // CHECK-NEXT: [[METHOD:%.*]] = witness_method $@opened("{{.*}}") InheritsMutatingMethod, #HasMutatingMethod.mutatingCounter!getter.1 : <Self where Self : HasMutatingMethod> (Self) -> () -> Value, [[X_PAYLOAD]] : $@opened("{{.*}}") InheritsMutatingMethod : $@convention(witness_method: HasMutatingMethod) <τ_0_0 where τ_0_0 : HasMutatingMethod> (@in_guaranteed τ_0_0) -> Value
   // CHECK-NEXT: [[RESULT_VALUE:%.*]] = apply [[METHOD]]<@opened("{{.*}}") InheritsMutatingMethod>([[TEMPORARY_2]]) : $@convention(witness_method: HasMutatingMethod) <τ_0_0 where τ_0_0 : HasMutatingMethod> (@in_guaranteed τ_0_0) -> Value
-  // CHECK-NEXT: destroy_addr [[TEMPORARY_2]] : $*@opened("{{.*}}") InheritsMutatingMethod
   // CHECK-NEXT: dealloc_stack  [[TEMPORARY_2]]
-  // CHECK-NEXT: end_access [[X_ADDR]] : $*InheritsMutatingMethod
+  // CHECK-NEXT: end_borrow [[X_PAYLOAD_RELOADED]]
   // CHECK-NEXT: assign [[RESULT_VALUE]] to [[RESULT]] : $*Value
+  // CHECK-NEXT: destroy_addr [[TEMPORARY]]
+  // CHECK-NEXT: end_access [[X_ADDR]]
   // CHECK-NEXT: dealloc_stack [[TEMPORARY]] : $*@opened("{{.*}}") InheritsMutatingMethod
   // CHECK-NEXT: dealloc_stack [[RESULT_BOX]] : $*Value
   _ = x.mutatingCounter

test/SILGen/properties.swift

@@ -1190,12 +1190,12 @@ protocol NonmutatingProtocol {
 // CHECK-NEXT:   [[C_FIELD_PAYLOAD:%.*]] = open_existential_addr immutable_access [[C_FIELD_BOX]] : $*NonmutatingProtocol to $*@opened("{{.*}}") NonmutatingProtocol
 // CHECK-NEXT:   [[C_FIELD_COPY:%.*]] = alloc_stack $@opened("{{.*}}") NonmutatingProtocol
 // CHECK-NEXT:   copy_addr [[C_FIELD_PAYLOAD]] to [initialization] [[C_FIELD_COPY]] : $*@opened("{{.*}}") NonmutatingProtocol
-// CHECK-NEXT:   destroy_addr [[C_FIELD_BOX]] : $*NonmutatingProtocol
-// CHECK-NEXT:   destroy_value [[C]] : $ReferenceType
 // CHECK-NEXT:   [[GETTER:%.*]] = witness_method $@opened("{{.*}}") NonmutatingProtocol, #NonmutatingProtocol.x!getter.1 : <Self where Self : NonmutatingProtocol> (Self) -> () -> Int, [[C_FIELD_PAYLOAD]] : $*@opened("{{.*}}") NonmutatingProtocol : $@convention(witness_method: NonmutatingProtocol) <τ_0_0 where τ_0_0 : NonmutatingProtocol> (@in_guaranteed τ_0_0) -> Int
 // CHECK-NEXT:   [[RESULT_VALUE:%.*]] = apply [[GETTER]]<@opened("{{.*}}") NonmutatingProtocol>([[C_FIELD_COPY]]) : $@convention(witness_method: NonmutatingProtocol) <τ_0_0 where τ_0_0 : NonmutatingProtocol> (@in_guaranteed τ_0_0) -> Int
 // CHECK-NEXT:   assign [[RESULT_VALUE]] to [[UNINIT]] : $*Int
 // CHECK-NEXT:   destroy_addr [[C_FIELD_COPY]] : $*@opened("{{.*}}") NonmutatingProtocol
+// CHECK-NEXT:   destroy_addr [[C_FIELD_BOX]] : $*NonmutatingProtocol
+// CHECK-NEXT:   destroy_value [[C]] : $ReferenceType
 // CHECK-NEXT:   dealloc_stack [[C_FIELD_COPY]] : $*@opened("{{.*}}") NonmutatingProtocol
 // CHECK-NEXT:   dealloc_stack [[C_FIELD_BOX]] : $*NonmutatingProtocol
 // CHECK-NEXT:   dealloc_stack [[RESULT]] : $*Int

test/SILGen/protocols.swift

@@ -26,10 +26,10 @@ func use_subscript_rvalue_get(_ i : Int) -> Int {
 // CHECK: [[PROJ:%[0-9]+]] = open_existential_addr immutable_access [[READ]] : $*SubscriptableGet to $*[[OPENED:@opened(.*) SubscriptableGet]]
 // CHECK: [[ALLOCSTACK:%[0-9]+]] = alloc_stack $[[OPENED]]
 // CHECK: copy_addr [[PROJ]] to [initialization] [[ALLOCSTACK]] : $*[[OPENED]]
-// CHECK-NEXT: end_access [[READ]] : $*SubscriptableGet
 // CHECK-NEXT: [[METH:%[0-9]+]] = witness_method $[[OPENED]], #SubscriptableGet.subscript!getter.1
 // CHECK-NEXT: [[RESULT:%[0-9]+]] = apply [[METH]]<[[OPENED]]>(%0, [[ALLOCSTACK]])
 // CHECK-NEXT: destroy_addr [[ALLOCSTACK]]
+// CHECK-NEXT: end_access [[READ]] : $*SubscriptableGet
 // CHECK-NEXT: dealloc_stack [[ALLOCSTACK]] : $*[[OPENED]]
 // CHECK-NEXT: return [[RESULT]]
 
@@ -133,9 +133,9 @@ func use_property_rvalue_get() -> Int {
 // CHECK: [[PROJ:%[0-9]+]] = open_existential_addr immutable_access [[READ]] : $*PropertyWithGetter to $*[[OPENED:@opened(.*) PropertyWithGetter]]
 // CHECK: [[COPY:%.*]] = alloc_stack $[[OPENED]]
 // CHECK-NEXT: copy_addr [[PROJ]] to [initialization] [[COPY]] : $*[[OPENED]]
-// CHECK-NEXT: end_access [[READ]] : $*PropertyWithGetter
 // CHECK-NEXT: [[METH:%[0-9]+]] = witness_method $[[OPENED]], #PropertyWithGetter.a!getter.1
 // CHECK-NEXT: apply [[METH]]<[[OPENED]]>([[COPY]])
+// CHECK: end_access [[READ]] : $*PropertyWithGetter
 
 func use_property_lvalue_get() -> Int {
   return propertyGetSet.b
@@ -458,3 +458,15 @@ public func test(_ p: Proto) {
 // CHECK-LABEL: sil_witness_table hidden StructWithStoredClassProperty: PropertyWithGetter module protocols {
 // CHECK-NEXT:  method #PropertyWithGetter.a!getter.1: {{.*}} : @$S9protocols29StructWithStoredClassPropertyVAA0fC6GetterA2aDP1aSivgTW
 // CHECK-NEXT: }
+
+//
+// rdar://problem/37031037
+//
+
+protocol MethodWithDefaultArgGenerator {}
+extension MethodWithDefaultArgGenerator {
+  mutating func foo(_ x: Int = 0) {}
+}
+func invokeMethodWithDefaultArg(x: inout MethodWithDefaultArgGenerator) {
+  x.foo()
+}