Merge pull request #22253 from eeckstein/fix-temprvalue-opt-5.0

[5.0] SILOptimizer: fix a bug in the TempRValue optimization which causes a miscompile.

Author: eeckstein

include/swift/SIL/SILArgumentConvention.h

@@ -87,6 +87,24 @@ struct SILArgumentConvention {
     return Value <= SILArgumentConvention::Indirect_Out;
   }
 
+  bool isInoutConvention() const {
+    switch (Value) {
+      case SILArgumentConvention::Indirect_Inout:
+      case SILArgumentConvention::Indirect_InoutAliasable:
+        return true;
+      case SILArgumentConvention::Indirect_In_Guaranteed:
+      case SILArgumentConvention::Indirect_In:
+      case SILArgumentConvention::Indirect_In_Constant:
+      case SILArgumentConvention::Indirect_Out:
+      case SILArgumentConvention::Direct_Unowned:
+      case SILArgumentConvention::Direct_Owned:
+      case SILArgumentConvention::Direct_Deallocating:
+      case SILArgumentConvention::Direct_Guaranteed:
+        return false;
+    }
+    llvm_unreachable("covered switch isn't covered?!");
+  }
+
   bool isOwnedConvention() const {
     switch (Value) {
     case SILArgumentConvention::Indirect_In:

lib/SILOptimizer/Transforms/CopyForwarding.cpp

@@ -1555,8 +1555,9 @@ class CopyForwardingPass : public SILFunctionTransform
 class TempRValueOptPass : public SILFunctionTransform {
   AliasAnalysis *AA = nullptr;
 
-  static bool collectLoads(Operand *UserOp, SILInstruction *UserInst,
+  bool collectLoads(Operand *UserOp, SILInstruction *UserInst,
                            SingleValueInstruction *Addr,
+                           SILValue srcObject,
                            llvm::SmallPtrSetImpl<SILInstruction *> &LoadInsts);
 
   bool checkNoSourceModification(CopyAddrInst *copyInst,
@@ -1615,6 +1616,7 @@ void TempRValueOptPass::run() {
 /// reaching a load or returning false.
 bool TempRValueOptPass::collectLoads(
     Operand *userOp, SILInstruction *user, SingleValueInstruction *address,
+    SILValue srcObject,
     llvm::SmallPtrSetImpl<SILInstruction *> &loadInsts) {
   // All normal uses (loads) must be in the initialization block.
   // (The destroy and dealloc are commonly in a different block though.)
@@ -1639,22 +1641,42 @@ bool TempRValueOptPass::collectLoads(
 
   case SILInstructionKind::ApplyInst: {
     ApplySite apply(user);
+
+    // Check if the function can just read from userOp.
     auto Convention = apply.getArgumentConvention(*userOp);
-    if (Convention.isGuaranteedConvention()) {
-      loadInsts.insert(user);
-      return true;
+    if (!Convention.isGuaranteedConvention()) {
+      LLVM_DEBUG(llvm::dbgs() << "  Temp consuming use may write/destroy "
+                 "its source" << *user);
+      return false;
     }
-    LLVM_DEBUG(llvm::dbgs() << "  Temp consuming use may write/destroy "
-                               "its source"
-                            << *user);
-    return false;
+
+    // Check if there is another function argument, which is inout which might
+    // modify the source of the copy_addr.
+    // If we would remove the copy_addr in this case, it would result in an
+    // exclusivity violation.
+    auto calleeConv = apply.getSubstCalleeConv();
+    unsigned calleeArgIdx = apply.getCalleeArgIndexOfFirstAppliedArg();
+    for (Operand &operand : apply.getArgumentOperands()) {
+      auto argConv = calleeConv.getSILArgumentConvention(calleeArgIdx);
+      if (argConv.isInoutConvention()) {
+        if (!AA->isNoAlias(operand.get(), srcObject)) {
+          return false;
+        }
+      }
+      ++calleeArgIdx;
+    }
+
+    // Everything is okay with the function call. Register it as a "load".
+    loadInsts.insert(user);
+    return true;
   }
   case SILInstructionKind::StructElementAddrInst:
   case SILInstructionKind::TupleElementAddrInst: {
     // Transitively look through projections on stack addresses.
     auto proj = cast<SingleValueInstruction>(user);
     for (auto *projUseOper : proj->getUses()) {
-      if (!collectLoads(projUseOper, projUseOper->getUser(), proj, loadInsts))
+      if (!collectLoads(projUseOper, projUseOper->getUser(), proj, srcObject,
+                        loadInsts))
         return false;
     }
     return true;
@@ -1744,7 +1766,7 @@ bool TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {
     if (isa<DestroyAddrInst>(user) || isa<DeallocStackInst>(user))
       continue;
 
-    if (!collectLoads(useOper, user, tempObj, loadInsts))
+    if (!collectLoads(useOper, user, tempObj, copyInst->getSrc(), loadInsts))
       return false;
   }
 

test/SILOptimizer/temp_rvalue_opt.sil

@@ -444,3 +444,30 @@ bb0(%0 : $*Klass):
   %9999 = tuple()
   return %9999 : $()
 }
+
+sil @$createKlass : $@convention(thin) () -> @out Klass
+sil @$appendKlass : $@convention(method) (@in_guaranteed Klass, @inout Klass) -> ()
+
+// CHECK-LABEL: sil @$overlapping_lifetime_in_function_all : $@convention(thin) () -> @out Klass {
+// CHECK: [[S1:%.*]] = alloc_stack $Klass
+// CHECK: [[S2:%.*]] = alloc_stack $Klass
+// CHECK: copy_addr [[S1]] to [initialization] [[S2]]
+// CHECK: apply {{%.*}}([[S2]], [[S1]])
+// CHECK: }
+sil @$overlapping_lifetime_in_function_all : $@convention(thin) () -> @out Klass {
+bb0(%0 : $*Klass):
+  %1 = alloc_stack $Klass
+  %2 = function_ref @$createKlass : $@convention(thin) () -> @out Klass
+  %3 = apply %2(%1) : $@convention(thin) () -> @out Klass
+  %4 = alloc_stack $Klass
+  copy_addr %1 to [initialization] %4 : $*Klass
+  %6 = function_ref @$appendKlass : $@convention(method) (@in_guaranteed Klass, @inout Klass) -> ()
+  %7 = apply %6(%4, %1) : $@convention(method) (@in_guaranteed Klass, @inout Klass) -> ()
+  destroy_addr %4 : $*Klass
+  dealloc_stack %4 : $*Klass
+  copy_addr [take] %1 to [initialization] %0 : $*Klass
+  dealloc_stack %1 : $*Klass
+  %12 = tuple ()
+  return %12 : $()
+}
+