Merge pull request #63255 from gottesmm/pr-f6f932ba70eaa205c45042884148ff1033d9a416

[move-only] Make sure that we insert compensating destroys for phi values

Author: gottesmm

lib/SILOptimizer/Mandatory/MoveOnlyObjectChecker.h

@@ -177,13 +177,12 @@ struct BorrowToDestructureTransform {
   DiagnosticEmitter &diagnosticEmitter;
   FieldSensitiveSSAPrunedLiveRange liveness;
   SmallVector<Operand *, 8> destructureNeedingUses;
-  SmallVector<Operand *, 8> livenessNeedingUses;
-  SmallVector<SILInstruction *, 8> endScopeInsts;
   PostOrderAnalysis *poa;
   PostOrderFunctionInfo *pofi = nullptr;
   Optional<AvailableValueStore> blockToAvailableValues;
   SILValue initialValue;
   SmallVector<SILInstruction *, 8> createdDestructures;
+  SmallVector<SILPhiArgument *, 8> createdPhiArguments;
 
   using InterestingUser = FieldSensitivePrunedLiveness::InterestingUser;
   SmallFrozenMultiMap<SILBasicBlock *, std::pair<Operand *, InterestingUser>, 8>

lib/SILOptimizer/Mandatory/MoveOnlyUtils.cpp

@@ -135,7 +135,6 @@ bool BorrowToDestructureTransform::gatherUses(
       blocksToUses.insert(
           nextUse->getParentBlock(),
           {nextUse, {*leafRange, false /*is lifetime ending*/}});
-      livenessNeedingUses.push_back(nextUse);
       liveness.updateForUse(nextUse->getUser(), *leafRange,
                             false /*is lifetime ending*/);
       continue;
@@ -146,7 +145,6 @@ bool BorrowToDestructureTransform::gatherUses(
       // Ignore destroy_value, we are going to eliminate them.
       if (isa<DestroyValueInst>(nextUse->getUser())) {
         LLVM_DEBUG(llvm::dbgs() << "        Found destroy value!\n");
-        endScopeInsts.push_back(nextUse->getUser());
         continue;
       }
 
@@ -186,7 +184,6 @@ bool BorrowToDestructureTransform::gatherUses(
       continue;
     case OperandOwnership::EndBorrow:
       LLVM_DEBUG(llvm::dbgs() << "        Found end borrow!\n");
-      endScopeInsts.push_back(nextUse->getUser());
       continue;
     case OperandOwnership::Reborrow:
       llvm_unreachable("Unsupported for now?!");
@@ -879,8 +876,12 @@ BorrowToDestructureTransform::computeAvailableValues(SILBasicBlock *block) {
     }
 
     // Ok, we need to actually construct a phi.
-    SILType offsetType = smallestTypeAvailable[i]->second;
-    newValues[i] = block->createPhiArgument(offsetType, OwnershipKind::Owned);
+    {
+      SILType offsetType = smallestTypeAvailable[i]->second;
+      auto *phi = block->createPhiArgument(offsetType, OwnershipKind::Owned);
+      newValues[i] = phi;
+      createdPhiArguments.push_back(phi);
+    }
 
     for (auto *predBlock : predsSkippingBackEdges) {
       auto &predAvailableValues = computeAvailableValues(predBlock);
@@ -1299,6 +1300,23 @@ void BorrowToDestructureTransform::cleanup(
     }
   }
 
+  // Then do this for our inserted phis.
+  while (!createdPhiArguments.empty()) {
+    auto *arg = createdPhiArguments.pop_back_val();
+
+    // If we have a trivial argument, we do not ened to add any compensating
+    // destroys.
+    if (arg->getType().isTrivial(*fn))
+      continue;
+
+    SWIFT_DEFER {
+      liveness.clear();
+      discoveredBlocks.clear();
+      boundary.clear();
+    };
+    addCompensatingDestroys(liveness, boundary, arg);
+  }
+
   // And finally do the same thing for our initial copy_value.
   addCompensatingDestroys(liveness, boundary, initialValue);
 }

test/SILOptimizer/moveonly_borrow_to_destructure_transform.sil

@@ -25,12 +25,29 @@ struct AggStruct {
   var pair: KlassPair
 }
 
+@_moveOnly
+struct KlassPair2 {
+  var lhs: MoveOnlyKlass
+  var rhs: MoveOnlyKlass
+}
+
+@_moveOnly
+struct AggStruct2 {
+  var lhs: MoveOnlyKlass
+  var pair: KlassPair2
+  var rhs: MoveOnlyKlass
+}
+
 @_moveOnly
 struct SingleIntContainingStruct {
     var value: Builtin.Int32
 }
 
 sil @misc_use : $@convention(thin) () -> ()
+sil @aggstruct_consume : $@convention(thin) (@owned AggStruct) -> ()
+sil @moveonlyklass_consume : $@convention(thin) (@owned MoveOnlyKlass) -> ()
+sil @moveonlyklass_use : $@convention(thin) (@guaranteed MoveOnlyKlass) -> ()
+sil @klass_use : $@convention(thin) (@guaranteed Klass) -> ()
 
 ///////////
 // Tests //
@@ -137,6 +154,61 @@ bb0(%0 : @guaranteed $SingleIntContainingStruct):
   return %5 : $Builtin.Int32
 }
 
+// CHECK-LABEL: sil [ossa] @phis_are_cleaned_up : $@convention(thin) (@owned AggStruct2) -> () {
+// CHECK: bb3([[FIRST_PHI:%.*]] : @owned $MoveOnlyKlass, [[SECOND_PHI:%.*]] : @owned $MoveOnlyKlass, [[THIRD_PHI:%.*]] : @owned $MoveOnlyKlass):
+// CHECK:   destroy_value [[SECOND_PHI]]
+// CHECK:   destroy_value [[THIRD_PHI]]
+// CHECK:   apply {{%.*}}([[FIRST_PHI]])
+// CHECK:   destroy_value [[FIRST_PHI]]
+// CHECK: } // end sil function 'phis_are_cleaned_up'
+sil [ossa] @phis_are_cleaned_up : $@convention(thin) (@owned AggStruct2) -> () {
+bb0(%0 : @owned $AggStruct2):
+  %1 = move_value [lexical] %0 : $AggStruct2
+  %2 = mark_must_check [no_implicit_copy] %1 : $AggStruct2
+  debug_value %2 : $AggStruct2, let, name "x2", argno 1
+  cond_br undef, bb1, bb2
+
+bb1:
+  %8 = begin_borrow %2 : $AggStruct2
+  %9 = struct_extract %8 : $AggStruct2, #AggStruct2.pair
+  %10 = copy_value %9 : $KlassPair2
+  %11 = begin_borrow %10 : $KlassPair2
+  %12 = struct_extract %11 : $KlassPair2, #KlassPair2.rhs
+  %13 = copy_value %12 : $MoveOnlyKlass
+  end_borrow %11 : $KlassPair2
+  destroy_value %10 : $KlassPair2
+  %16 = function_ref @moveonlyklass_consume : $@convention(thin) (@owned MoveOnlyKlass) -> ()
+  %17 = apply %16(%13) : $@convention(thin) (@owned MoveOnlyKlass) -> ()
+  end_borrow %8 : $AggStruct2
+  br bb3
+
+bb2:
+  %20 = begin_borrow %2 : $AggStruct2
+  %21 = struct_extract %20 : $AggStruct2, #AggStruct2.pair
+  %22 = copy_value %21 : $KlassPair2
+  %23 = begin_borrow %22 : $KlassPair2
+  %24 = struct_extract %23 : $KlassPair2, #KlassPair2.rhs
+  %25 = copy_value %24 : $MoveOnlyKlass
+  end_borrow %23 : $KlassPair2
+  destroy_value %22 : $KlassPair2
+  %28 = function_ref @moveonlyklass_consume : $@convention(thin) (@owned MoveOnlyKlass) -> ()
+  %29 = apply %28(%25) : $@convention(thin) (@owned MoveOnlyKlass) -> ()
+  end_borrow %20 : $AggStruct2
+  br bb3
+
+bb3:
+  %32 = begin_borrow %2 : $AggStruct2
+  %33 = struct_extract %32 : $AggStruct2, #AggStruct2.lhs
+  %34 = copy_value %33 : $MoveOnlyKlass
+  %35 = function_ref @moveonlyklass_use : $@convention(thin) (@guaranteed MoveOnlyKlass) -> ()
+  %36 = apply %35(%34) : $@convention(thin) (@guaranteed MoveOnlyKlass) -> ()
+  destroy_value %34 : $MoveOnlyKlass
+  end_borrow %32 : $AggStruct2
+  destroy_value %2 : $AggStruct2
+  %39 = tuple ()
+  return %39 : $()
+}
+
 ///////////////////////////////
 // Single Field Struct Tests //
 ///////////////////////////////