Emit bind optional fixes

lib/SILGen/SILGenConvert.cpp

@@ -391,9 +391,8 @@ SILGenFunction::emitOptionalToOptional(SILLocation loc,
     finalResult = B.createOwnedPHIArgument(resultTL.getLoweredType());
   }
 
-  SEBuilder.addCase(
-      getASTContext().getOptionalSomeDecl(), isPresentBB, contBB,
-      [&](ManagedValue input, SwitchCaseFullExpr &scope) {
+  SEBuilder.addOptionalSomeCase(
+      isPresentBB, contBB, [&](ManagedValue input, SwitchCaseFullExpr &&scope) {
         // If we have an address only type, we want to match the old behavior of
         // transforming the underlying type instead of the optional type. This
         // ensures that we use the more efficient non-generic code paths when
@@ -420,9 +419,9 @@ SILGenFunction::emitOptionalToOptional(SILLocation loc,
         return scope.exitAndBranch(loc);
       });
 
-  SEBuilder.addCase(
-      getASTContext().getOptionalNoneDecl(), isNotPresentBB, contBB,
-      [&](ManagedValue input, SwitchCaseFullExpr &scope) {
+  SEBuilder.addOptionalNoneCase(
+      isNotPresentBB, contBB,
+      [&](ManagedValue input, SwitchCaseFullExpr &&scope) {
         if (!(resultTL.isAddressOnly() && silConv.useLoweredAddresses())) {
           SILValue none =
               B.createManagedOptionalNone(loc, resultTy).forward(*this);

lib/SILGen/SILGenDecl.cpp

@@ -779,7 +779,7 @@ void EnumElementPatternInitialization::emitEnumMatch(
   // path, causing a use (the destroy on the negative path) to be created that
   // does not dominate its definition (in the positive path).
   auto handler = [&SGF, &loc, &failureDest](ManagedValue mv,
-                                            SwitchCaseFullExpr &expr) {
+                                            SwitchCaseFullExpr &&expr) {
     expr.exit();
     SGF.Cleanups.emitBranchAndCleanups(failureDest, loc);
   };
@@ -804,7 +804,7 @@ void EnumElementPatternInitialization::emitEnumMatch(
   switchBuilder.addCase(
       eltDecl, someBlock, contBlock,
       [&SGF, &loc, &eltDecl, &subInit, &value](ManagedValue mv,
-                                               SwitchCaseFullExpr &expr) {
+                                               SwitchCaseFullExpr &&expr) {
         // If the enum case has no bound value, we're done.
         if (!eltDecl->hasAssociatedValues()) {
           assert(

lib/SILGen/SILGenExpr.cpp

@@ -4253,24 +4253,24 @@ static ManagedValue flattenOptional(SILGenFunction &SGF, SILLocation loc,
 
   SwitchEnumBuilder SEB(SGF.B, loc, optVal);
 
-  auto *someDecl = SGF.getASTContext().getOptionalSomeDecl();
-  SEB.addCase(someDecl, isPresentBB, contBB, [&](ManagedValue input,
-                                                 SwitchCaseFullExpr &scope) {
-    if (resultTL.isAddressOnly()) {
-      SILValue addr =
-          addrOnlyResultBuf->getAddressForInPlaceInitialization(SGF, loc);
-      input = SGF.B.createUncheckedTakeEnumDataAddr(
-          loc, input, someDecl, input.getType().getOptionalObjectType());
-      SGF.B.createCopyAddr(loc, input.getValue(), addr, IsNotTake,
-                           IsInitialization);
-      scope.exitAndBranch(loc);
-      return;
-    }
-    scope.exitAndBranch(loc, input.forward(SGF));
-  });
-  SEB.addCase(
-      SGF.getASTContext().getOptionalNoneDecl(), isNotPresentBB, contBB,
-      [&](ManagedValue input, SwitchCaseFullExpr &scope) {
+  SEB.addOptionalSomeCase(
+      isPresentBB, contBB, [&](ManagedValue input, SwitchCaseFullExpr &&scope) {
+        if (resultTL.isAddressOnly()) {
+          SILValue addr =
+              addrOnlyResultBuf->getAddressForInPlaceInitialization(SGF, loc);
+          auto *someDecl = SGF.getASTContext().getOptionalSomeDecl();
+          input = SGF.B.createUncheckedTakeEnumDataAddr(
+              loc, input, someDecl, input.getType().getOptionalObjectType());
+          SGF.B.createCopyAddr(loc, input.getValue(), addr, IsNotTake,
+                               IsInitialization);
+          scope.exitAndBranch(loc);
+          return;
+        }
+        scope.exitAndBranch(loc, input.forward(SGF));
+      });
+  SEB.addOptionalNoneCase(
+      isNotPresentBB, contBB,
+      [&](ManagedValue input, SwitchCaseFullExpr &&scope) {
         if (resultTL.isAddressOnly()) {
           SILValue addr =
               addrOnlyResultBuf->getAddressForInPlaceInitialization(SGF, loc);
@@ -4896,24 +4896,32 @@ RValue RValueEmitter::visitAssignExpr(AssignExpr *E, SGFContext C) {
   return SGF.emitEmptyTupleRValue(E, C);
 }
 
-void SILGenFunction::emitBindOptional(SILLocation loc,
-                                      ManagedValue optionalAddrOrValue,
+void SILGenFunction::emitBindOptional(SILLocation loc, ManagedValue optValue,
                                       unsigned depth) {
   assert(depth < BindOptionalFailureDests.size());
   auto failureDest = BindOptionalFailureDests[BindOptionalFailureDests.size()
                                                 - depth - 1];
 
-  // Check whether the optional has a value.
   SILBasicBlock *hasValueBB = createBasicBlock();
-  auto hasValue =
-      emitDoesOptionalHaveValue(loc, optionalAddrOrValue.getValue());
-
+  SILBasicBlock *hasNoValueBB = createBasicBlock();
+
+  // We make a copy to ensure that we pass the value into the
+  // switch_enum at plus 1. This is important since emitBindOptional
+  // does not consume its argument. Additionally, we need to make sure
+  // to create the switch enum builder /after/ emitting the block for
+  // cleanups lest we emit an extra destroy in the .none block.
+  SwitchEnumBuilder SEB(B, loc, optValue.copy(*this, loc));
+  SEB.addOptionalSomeCase(hasValueBB);
   // If not, thread out through a bunch of cleanups.
-  SILBasicBlock *hasNoValueBB = Cleanups.emitBlockForCleanups(failureDest, loc);
-  B.createCondBranch(loc, hasValue, hasValueBB, hasNoValueBB);
-
-  // If so, continue.
-  B.emitBlock(hasValueBB);
+  SEB.addOptionalNoneCase(hasNoValueBB, failureDest,
+                          [&](ManagedValue mv, SwitchCaseFullExpr &&expr) {
+                            expr.exitAndBranch(loc);
+                          });
+  std::move(SEB).emit();
+
+  // Reset the insertion point at the end of hasValueBB so we can
+  // continue to emit code there.
+  B.setInsertionPoint(hasValueBB);
 }
 
 RValue RValueEmitter::visitBindOptionalExpr(BindOptionalExpr *E, SGFContext C) {

lib/SILGen/SILGenStmt.cpp

@@ -816,10 +816,9 @@ void StmtEmitter::visitForEachStmt(ForEachStmt *S) {
   SILBasicBlock *failExitingBlock = createBasicBlock();
   SwitchEnumBuilder switchEnumBuilder(SGF.B, S, nextBufOrValue);
 
-  switchEnumBuilder.addCase(
-      SGF.getASTContext().getOptionalSomeDecl(), createBasicBlock(),
-      loopDest.getBlock(),
-      [&](ManagedValue inputValue, SwitchCaseFullExpr &scope) {
+  switchEnumBuilder.addOptionalSomeCase(
+      createBasicBlock(), loopDest.getBlock(),
+      [&](ManagedValue inputValue, SwitchCaseFullExpr &&scope) {
         SGF.emitProfilerIncrement(S->getBody());
 
         // Emit the loop body.
@@ -868,8 +867,10 @@ void StmtEmitter::visitForEachStmt(ForEachStmt *S) {
 
         // If we emitted an unreachable in the body, we will not have a valid
         // insertion point. Just return early.
-        if (!SGF.B.hasValidInsertionPoint())
+        if (!SGF.B.hasValidInsertionPoint()) {
+          scope.unreachableExit();
           return;
+        }
 
         // Otherwise, associate the loop body's closing brace with this branch.
         RegularLocation L(S->getBody());
@@ -881,10 +882,9 @@ void StmtEmitter::visitForEachStmt(ForEachStmt *S) {
   // We add loop fail block, just to be defensive about intermediate
   // transformations performing cleanups at scope.exit(). We still jump to the
   // contBlock.
-  switchEnumBuilder.addCase(
-      SGF.getASTContext().getOptionalNoneDecl(), createBasicBlock(),
-      failExitingBlock,
-      [&](ManagedValue inputValue, SwitchCaseFullExpr &scope) {
+  switchEnumBuilder.addOptionalNoneCase(
+      createBasicBlock(), failExitingBlock,
+      [&](ManagedValue inputValue, SwitchCaseFullExpr &&scope) {
         assert(!inputValue && "None should not be passed an argument!");
         scope.exitAndBranch(S);
       },

lib/SILGen/SwitchEnumBuilder.cpp

@@ -21,26 +21,47 @@ using namespace Lowering;
 //                     SwitchCaseFullExpr Implementation
 //===----------------------------------------------------------------------===//
 
+SwitchCaseFullExpr::SwitchCaseFullExpr(SILGenFunction &SGF, CleanupLocation loc)
+    : SGF(SGF), scope(SGF.Cleanups, loc), loc(loc), branchDest() {}
+
 SwitchCaseFullExpr::SwitchCaseFullExpr(SILGenFunction &SGF, CleanupLocation loc,
-                                       SILBasicBlock *contBlock)
-    : SGF(SGF), scope(SGF.Cleanups, loc), loc(loc), contBlock(contBlock) {}
+                                       SwitchCaseBranchDest branchDest)
+    : SGF(SGF), scope(SGF.Cleanups, loc), loc(loc), branchDest(branchDest) {}
 
 void SwitchCaseFullExpr::exitAndBranch(SILLocation loc,
                                        ArrayRef<SILValue> branchArgs) {
-  assert(contBlock &&
-         "Should not call this if we do not have a continuation block");
+  assert(bool(branchDest) && "Must have a branch destination!");
   assert(SGF.B.hasValidInsertionPoint());
   scope.pop();
-  SGF.B.createBranch(loc, contBlock.get(), branchArgs);
+
+  // Then either do a direct branch or a branch + cleanups.
+  if (SILBasicBlock *block = branchDest.getBlock()) {
+    SGF.B.createBranch(loc, block, branchArgs);
+    return;
+  }
+
+  SGF.Cleanups.emitBranchAndCleanups(branchDest.getJumpDest(), loc, branchArgs);
 }
 
 void SwitchCaseFullExpr::exit() {
-  assert(!contBlock &&
+  assert(!bool(branchDest) &&
          "Should not call this if we do have a continuation block");
   assert(SGF.B.hasValidInsertionPoint());
   scope.pop();
 }
 
+SwitchCaseFullExpr::~SwitchCaseFullExpr() {
+  assert(!scope.isValid() && "Did not pop scope?!");
+}
+
+void SwitchCaseFullExpr::unreachableExit() {
+  // This is important to ensure that we do not actually emit any cleanups since
+  // we already know that an unreachable was emitted.
+  assert(!SGF.B.hasValidInsertionPoint() && "Expected to pop scope without a "
+                                            "valid insertion point!");
+  scope.pop();
+}
+
 //===----------------------------------------------------------------------===//
 //                      SwitchEnumBuilder Implementation
 //===----------------------------------------------------------------------===//
@@ -93,32 +114,30 @@ void SwitchEnumBuilder::emit() && {
       defaultBlockData->dispatchTime ==
           DefaultDispatchTime::BeforeNormalCases) {
     SILBasicBlock *defaultBlock = defaultBlockData->block;
-    NullablePtr<SILBasicBlock> contBB = defaultBlockData->contBlock;
+    SwitchCaseBranchDest branchDest = defaultBlockData->branchDest;
     DefaultCaseHandler handler = defaultBlockData->handler;
 
     // Don't allow cleanups to escape the conditional block.
     SwitchCaseFullExpr presentScope(builder.getSILGenFunction(),
-                                    CleanupLocation::get(loc),
-                                    contBB.getPtrOrNull());
+                                    CleanupLocation::get(loc), branchDest);
     builder.emitBlock(defaultBlock);
     ManagedValue input = optional;
     if (!isAddressOnly) {
       input = builder.createOwnedPHIArgument(optional.getType());
     }
-    handler(input, presentScope);
+    handler(input, std::move(presentScope));
     builder.clearInsertionPoint();
   }
 
   for (NormalCaseData &caseData : caseDataArray) {
     EnumElementDecl *decl = caseData.decl;
     SILBasicBlock *caseBlock = caseData.block;
-    NullablePtr<SILBasicBlock> contBlock = caseData.contBlock;
+    SwitchCaseBranchDest branchDest = caseData.branchDest;
     NormalCaseHandler handler = caseData.handler;
 
     // Don't allow cleanups to escape the conditional block.
     SwitchCaseFullExpr presentScope(builder.getSILGenFunction(),
-                                    CleanupLocation::get(loc),
-                                    contBlock.getPtrOrNull());
+                                    CleanupLocation::get(loc), branchDest);
 
     builder.emitBlock(caseBlock);
 
@@ -132,7 +151,7 @@ void SwitchEnumBuilder::emit() && {
         input = builder.createOwnedPHIArgument(inputType);
       }
     }
-    handler(input, presentScope);
+    handler(input, std::move(presentScope));
     builder.clearInsertionPoint();
   }
 
@@ -141,19 +160,18 @@ void SwitchEnumBuilder::emit() && {
   if (defaultBlockData &&
       defaultBlockData->dispatchTime == DefaultDispatchTime::AfterNormalCases) {
     SILBasicBlock *defaultBlock = defaultBlockData->block;
-    NullablePtr<SILBasicBlock> contBB = defaultBlockData->contBlock;
+    auto branchDest = defaultBlockData->branchDest;
     DefaultCaseHandler handler = defaultBlockData->handler;
 
     // Don't allow cleanups to escape the conditional block.
     SwitchCaseFullExpr presentScope(builder.getSILGenFunction(),
-                                    CleanupLocation::get(loc),
-                                    contBB.getPtrOrNull());
+                                    CleanupLocation::get(loc), branchDest);
     builder.emitBlock(defaultBlock);
     ManagedValue input = optional;
     if (!isAddressOnly) {
       input = builder.createOwnedPHIArgument(optional.getType());
     }
-    handler(input, presentScope);
+    handler(input, std::move(presentScope));
     builder.clearInsertionPoint();
   }
 }