Merge pull request #25693 from gottesmm/pr-2677daef7169119f14e1aaf6f7117ef39430deb8

[cast-opt] Improve handling of arguments when optimizing Swift -> Obj…

Author: gottesmm

include/swift/SIL/SILBuilder.h

@@ -2092,6 +2092,16 @@ class SILBuilder {
     lowering.emitDestroyValue(*this, Loc, v);
   }
 
+  /// Convenience function for destroying objects and addresses.
+  ///
+  /// Objects are destroyed using emitDestroyValueOperation and addresses by
+  /// emitting destroy_addr.
+  void emitDestroyOperation(SILLocation loc, SILValue v) {
+    if (v->getType().isObject())
+      return emitDestroyValueOperation(loc, v);
+    createDestroyAddr(loc, v);
+  }
+
   SILValue emitTupleExtract(SILLocation Loc, SILValue Operand, unsigned FieldNo,
                             SILType ResultTy) {
     // Fold tuple_extract(tuple(x,y,z),2)

lib/SILOptimizer/Utils/CastOptimizer.cpp

@@ -346,27 +346,27 @@ CastOptimizer::optimizeBridgedObjCToSwiftCast(SILDynamicCastInst dynamicCast) {
   // If we have guaranteed normal arguments, insert the destroy.
   //
   // TODO: Is it safe to just eliminate the initial retain?
-  Builder.emitDestroyValueOperation(Loc, srcArg);
+  Builder.emitDestroyOperation(Loc, srcArg);
 
   // If we have an unconditional_checked_cast_addr, return early. We do not need
   // to handle any conditional code.
   if (isa<UnconditionalCheckedCastAddrInst>(Inst)) {
     // Destroy the source value as unconditional_checked_cast_addr would.
-    Builder.emitDestroyValueOperation(Loc, srcOp);
+    Builder.emitDestroyOperation(Loc, srcOp);
     eraseInstAction(Inst);
     return (newI) ? newI : AI;
   }
 
   auto *CCABI = cast<CheckedCastAddrBranchInst>(Inst);
   switch (CCABI->getConsumptionKind()) {
   case CastConsumptionKind::TakeAlways:
-    Builder.emitDestroyValueOperation(Loc, srcOp);
+    Builder.emitDestroyOperation(Loc, srcOp);
     break;
   case CastConsumptionKind::TakeOnSuccess: {
     {
       // Insert a release in the success BB.
       SILBuilderWithScope successBuilder(SuccessBB->begin());
-      successBuilder.emitDestroyValueOperation(Loc, srcOp);
+      successBuilder.emitDestroyOperation(Loc, srcOp);
     }
     {
       // And a store in the failure BB.
@@ -590,7 +590,7 @@ static SILValue computeFinalCastedValue(SILBuilderWithScope &builder,
         return failureBB->createPhiArgument(newAI->getType(),
                                             ValueOwnershipKind::Owned);
       }());
-      innerBuilder.emitDestroyValueOperation(loc, valueToDestroy);
+      innerBuilder.emitDestroyOperation(loc, valueToDestroy);
     }
 
     auto *condBrSuccessBB =
@@ -651,23 +651,25 @@ CastOptimizer::optimizeBridgedSwiftToObjCCast(SILDynamicCastInst dynamicCast) {
   SILType SubstFnTy = bridgedFunc->getLoweredType().substGenericArgs(M, subMap);
   SILFunctionConventions substConv(SubstFnTy.castTo<SILFunctionType>(), M);
 
-  // check that we can go through with the optimization
+  // Check that this is a case that the authors of this code thought it could
+  // handle.
   if (!canOptimizeCast(BridgedTargetTy, M, substConv)) {
     return nullptr;
   }
 
   SILBuilderWithScope Builder(Inst, builderContext);
   auto FnRef = Builder.createFunctionRefFor(Loc, bridgedFunc);
   auto ParamTypes = SubstFnTy.castTo<SILFunctionType>()->getParameters();
+  SILValue oldSrc;
   if (Src->getType().isAddress() && !substConv.isSILIndirect(ParamTypes[0])) {
     // Create load
+    oldSrc = Src;
     Src =
         Builder.emitLoadValueOperation(Loc, Src, LoadOwnershipQualifier::Take);
   }
 
   // Compensate different owning conventions of the replaced cast instruction
   // and the inserted conversion function.
-  bool needRetainBeforeCall = false;
   bool needReleaseAfterCall = false;
   bool needReleaseInSuccess = false;
   switch (ParamTypes[0].getConvention()) {
@@ -681,21 +683,10 @@ CastOptimizer::optimizeBridgedSwiftToObjCCast(SILDynamicCastInst dynamicCast) {
       needReleaseInSuccess = true;
       break;
     case CastConsumptionKind::BorrowAlways:
+      llvm_unreachable("Should never hit this");
     case CastConsumptionKind::CopyOnSuccess:
-      // Conservatively insert a retain/release pair around the conversion
-      // function because the conversion function could decrement the
-      // (global) reference count of the source object.
-      //
-      // %src = load %global_var
-      // apply %conversion_func(@guaranteed %src)
-      //
-      // sil conversion_func {
-      //    %old_value = load %global_var
-      //    store %something_else, %global_var
-      //    strong_release %old_value
-      // }
-      needRetainBeforeCall = true;
-      needReleaseAfterCall = true;
+      // We assume that our caller is correct and will treat our argument as
+      // being immutable, so we do not need to do anything here.
       break;
     }
     break;
@@ -733,53 +724,35 @@ CastOptimizer::optimizeBridgedSwiftToObjCCast(SILDynamicCastInst dynamicCast) {
     return nullptr;
   }
 
-  bool needStackAllocatedTemporary = false;
-  if (needRetainBeforeCall) {
-    if (AddressOnlyType) {
-      needStackAllocatedTemporary = true;
-      auto NewSrc = Builder.createAllocStack(Loc, Src->getType());
-      Builder.createCopyAddr(Loc, Src, NewSrc, IsNotTake, IsInitialization);
-      Src = NewSrc;
-    } else {
-      Builder.createRetainValue(Loc, Src, Builder.getDefaultAtomicity());
-    }
-  }
-
   // Generate a code to invoke the bridging function.
   auto *NewAI = Builder.createApply(Loc, FnRef, subMap, Src);
 
-  auto releaseSrc = [&](SILBuilder &Builder) {
-    if (AddressOnlyType) {
-      Builder.createDestroyAddr(Loc, Src);
-    } else {
-      Builder.emitDestroyValueOperation(Loc, Src);
-    }
-  };
-
-  Optional<SILBuilder> SuccBuilder;
-  if (needReleaseInSuccess || needStackAllocatedTemporary)
-    SuccBuilder.emplace(SuccessBB->begin());
-
+  // First if we are going to destroy the value unconditionally, just insert the
+  // destroy right after the call. This handles some of the conditional cases
+  // and /all/ of the consuming unconditional cases.
   if (needReleaseAfterCall) {
-    releaseSrc(Builder);
-  } else if (needReleaseInSuccess) {
-    if (SuccessBB) {
-      releaseSrc(*SuccBuilder);
-    } else {
-      // For an unconditional cast, success is the only defined path
-      releaseSrc(Builder);
-    }
-  }
-
-  // Pop the temporary stack slot for a copied temporary.
-  if (needStackAllocatedTemporary) {
-    assert((bool)SuccessBB == (bool)FailureBB);
+    Builder.emitDestroyOperation(Loc, Src);
+  } else {
     if (SuccessBB) {
-      SuccBuilder->createDeallocStack(Loc, Src);
-      SILBuilderWithScope FailBuilder(&*FailureBB->begin(), Builder);
-      FailBuilder.createDeallocStack(Loc, Src);
+      SILBuilderWithScope succBuilder(&*SuccessBB->begin(), Builder);
+      if (needReleaseInSuccess) {
+        succBuilder.emitDestroyOperation(Loc, Src);
+      } else {
+        if (oldSrc) {
+          succBuilder.emitStoreValueOperation(Loc, Src, oldSrc,
+                                              StoreOwnershipQualifier::Init);
+        }
+      }
+      SILBuilderWithScope failBuilder(&*FailureBB->begin(), Builder);
+      if (oldSrc) {
+        failBuilder.emitStoreValueOperation(Loc, Src, oldSrc,
+                                            StoreOwnershipQualifier::Init);
+      }
     } else {
-      Builder.createDeallocStack(Loc, Src);
+      if (oldSrc) {
+        Builder.emitStoreValueOperation(Loc, Src, oldSrc,
+                                        StoreOwnershipQualifier::Init);
+      }
     }
   }