Merge pull request #64690 from gottesmm/pr-08a1f1872f4e90eb29be0ba7c845d07fbb008571

[move-only] Add a new type of mark_must_check initable_but_not_consumable

Author: jckarter

include/swift/SIL/SILInstruction.h

@@ -8318,8 +8318,8 @@ class MarkMustCheckInst
     ConsumableAndAssignable,
 
     /// A signal to the move only checker to perform no consume or assign
-    /// checking. This forces the result of this instruction owned value to never
-    /// be consumed (for let/var semantics) or assigned over (for var
+    /// checking. This forces the result of this instruction owned value to
+    /// never be consumed (for let/var semantics) or assigned over (for var
     /// semantics). Of course, we still allow for non-consuming uses.
     NoConsumeOrAssign,
 
@@ -8330,6 +8330,11 @@ class MarkMustCheckInst
     /// uninitialized state), but we are ok with the user assigning a new value,
     /// completely assigning over the value at once.
     AssignableButNotConsumable,
+
+    /// A signal to the move checker that the given value cannot be consumed or
+    /// assigned, but is allowed to be initialized. This is used for situations
+    /// like class initializers.
+    InitableButNotConsumable,
   };
 
 private:
@@ -8348,13 +8353,16 @@ class MarkMustCheckInst
 public:
   CheckKind getCheckKind() const { return kind; }
 
+  void setCheckKind(CheckKind newKind) { kind = newKind; }
+
   bool hasMoveCheckerKind() const {
     switch (kind) {
     case CheckKind::Invalid:
       return false;
     case CheckKind::ConsumableAndAssignable:
     case CheckKind::NoConsumeOrAssign:
     case CheckKind::AssignableButNotConsumable:
+    case CheckKind::InitableButNotConsumable:
       return true;
     }
   }

lib/SIL/IR/SILPrinter.cpp

@@ -2010,6 +2010,9 @@ class SILPrinter : public SILInstructionVisitor<SILPrinter> {
     case CheckKind::AssignableButNotConsumable:
       *this << "[assignable_but_not_consumable] ";
       break;
+    case CheckKind::InitableButNotConsumable:
+      *this << "[initable_but_not_consumable] ";
+      break;
     }
     *this << getIDAndType(I->getOperand());
   }

lib/SIL/Parser/ParseSIL.cpp

@@ -3712,7 +3712,10 @@ bool SILParser::parseSpecificSILInstruction(SILBuilder &B,
             .Case("consumable_and_assignable",
                   CheckKind::ConsumableAndAssignable)
             .Case("no_consume_or_assign", CheckKind::NoConsumeOrAssign)
-            .Case("assignable_but_not_consumable", CheckKind::AssignableButNotConsumable)
+            .Case("assignable_but_not_consumable",
+                  CheckKind::AssignableButNotConsumable)
+            .Case("initable_but_not_consumable",
+                  CheckKind::InitableButNotConsumable)
             .Default(CheckKind::Invalid);
 
     if (CKind == CheckKind::Invalid) {

lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp

@@ -2312,7 +2312,6 @@ void LifetimeChecker::updateInstructionForInitState(unsigned UseID) {
   // If this is an assign, rewrite it based on whether it is an initialization
   // or not.
   if (auto *AI = dyn_cast<AssignInst>(Inst)) {
-
     // Remove this instruction from our data structures, since we will be
     // removing it.
     Use.Inst = nullptr;
@@ -2328,6 +2327,24 @@ void LifetimeChecker::updateInstructionForInitState(unsigned UseID) {
                                 : AssignOwnershipQualifier::Reassign));
     }
 
+    // Look and see if we are assigning a moveonly type into a mark_must_check
+    // [assignable_but_not_consumable]. If we are, then we need to transition
+    // its flag to initable_but_not_assignable.
+    //
+    // NOTE: We should only ever have to do this for a single level since SILGen
+    // always initializes values completely and we enforce that invariant.
+    if (InitKind == IsInitialization) {
+      if (auto *mmci =
+              dyn_cast<MarkMustCheckInst>(stripAccessMarkers(AI->getDest()))) {
+        if (mmci->getCheckKind() ==
+                MarkMustCheckInst::CheckKind::AssignableButNotConsumable &&
+            isa<RefElementAddrInst>(stripAccessMarkers(mmci->getOperand()))) {
+          mmci->setCheckKind(
+              MarkMustCheckInst::CheckKind::InitableButNotConsumable);
+        }
+      }
+    }
+
     return;
   }
   if (auto *AI = dyn_cast<AssignByWrapperInst>(Inst)) {

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

@@ -500,8 +500,12 @@ static bool isInOutDefThatNeedsEndOfFunctionLiveness(MarkMustCheckInst *markedAd
 
     if (isa<GlobalAddrInst>(stripAccessMarkers(operand)))
       return true;
+
+  if (auto *rei = dyn_cast<RefElementAddrInst>(stripAccessMarkers(operand)))
+    return true;
   }
 
+
   return false;
 }
 
@@ -2159,6 +2163,19 @@ void MoveOnlyAddressCheckerPImpl::insertDestroysOnBoundary(
       insertUndefDebugValue(insertPt);
     } else {
       auto *inst = cast<SILInstruction>(defPair.first);
+
+      // If we have a dead def that is our mark must check and that mark must
+      // check was an init but not consumable, then do not destroy that
+      // def. This is b/c we are in some sort of class initialization and we are
+      // looking at the initial part of the live range before the initialization
+      // has occured. This is our way of makinmg this fit the model that the
+      // checker expects (which is that values are always initialized at the def
+      // point).
+      if (markedValue &&
+          markedValue->getCheckKind() ==
+              MarkMustCheckInst::CheckKind::InitableButNotConsumable)
+        continue;
+
       auto *insertPt = inst->getNextInstruction();
       assert(insertPt && "def instruction was a terminator");
       insertDestroyBeforeInstruction(addressUseState, insertPt,

lib/Serialization/DeserializeSIL.cpp

@@ -2200,17 +2200,6 @@ bool SILDeserializer::readSILInstruction(SILFunction *Fn,
     break;
   }
 
-  case SILInstructionKind::MarkMustCheckInst: {
-    using CheckKind = MarkMustCheckInst::CheckKind;
-    auto Ty = MF->getType(TyID);
-    auto CKind = CheckKind(Attr);
-    ResultInst = Builder.createMarkMustCheckInst(
-        Loc,
-        getLocalValue(ValID, getSILType(Ty, (SILValueCategory)TyCategory, Fn)),
-        CKind);
-    break;
-  }
-
   case SILInstructionKind::MarkUnresolvedReferenceBindingInst: {
     using Kind = MarkUnresolvedReferenceBindingInst::Kind;
     auto ty = MF->getType(TyID);
@@ -2288,6 +2277,16 @@ bool SILDeserializer::readSILInstruction(SILFunction *Fn,
     ResultInst = Builder.createMarkUninitialized(Loc, Val, Kind);
     break;
   }
+  case SILInstructionKind::MarkMustCheckInst: {
+    using CheckKind = MarkMustCheckInst::CheckKind;
+    auto Ty = MF->getType(TyID);
+    auto CKind = CheckKind(Attr);
+    ResultInst = Builder.createMarkMustCheckInst(
+        Loc,
+        getLocalValue(ValID, getSILType(Ty, (SILValueCategory)TyCategory, Fn)),
+        CKind);
+    break;
+  }
   case SILInstructionKind::StoreInst: {
     auto Ty = MF->getType(TyID);
     SILType addrType = getSILType(Ty, (SILValueCategory)TyCategory, Fn);

lib/Serialization/SerializeSIL.cpp

@@ -1482,7 +1482,6 @@ void SILSerializer::writeSILInstruction(const SILInstruction &SI) {
   case SILInstructionKind::CopyValueInst:
   case SILInstructionKind::ExplicitCopyValueInst:
   case SILInstructionKind::MoveValueInst:
-  case SILInstructionKind::MarkMustCheckInst:
   case SILInstructionKind::MarkUnresolvedReferenceBindingInst:
   case SILInstructionKind::MoveOnlyWrapperToCopyableValueInst:
   case SILInstructionKind::CopyableToMoveOnlyWrapperValueInst:
@@ -1561,6 +1560,11 @@ void SILSerializer::writeSILInstruction(const SILInstruction &SI) {
     writeOneOperandLayout(SI.getKind(), Attr, SI.getOperand(0));
     break;
   }
+  case SILInstructionKind::MarkMustCheckInst: {
+    unsigned Attr = unsigned(cast<MarkMustCheckInst>(&SI)->getCheckKind());
+    writeOneOperandExtraAttributeLayout(SI.getKind(), Attr, SI.getOperand(0));
+    break;
+  }
   case SILInstructionKind::MarkUninitializedInst: {
     unsigned Attr =
         (unsigned)cast<MarkUninitializedInst>(&SI)->getMarkUninitializedKind();

test/Interpreter/moveonly.swift

@@ -10,40 +10,82 @@ var Tests = TestSuite("MoveOnlyTests")
 
 @_moveOnly
 struct FD {
-    var a  = LifetimeTracked(0)
+  var a = LifetimeTracked(0)
 
-    deinit {
-    }
+  deinit {
+  }
 }
 
 Tests.test("simple deinit called once") {
-    do {
-        let s = FD()
-    }
-    expectEqual(0, LifetimeTracked.instances)
+  do {
+    let s = FD()
+  }
+  expectEqual(0, LifetimeTracked.instances)
 }
 
 Tests.test("ref element addr destroyed once") {
-    class CopyableKlass {
-        var fd = FD()
-    }
+  class CopyableKlass {
+    var fd = FD()
+  }
 
-    func assignCopyableKlass(_ x: CopyableKlass) {
-        x.fd = FD()
-    }
+  func assignCopyableKlass(_ x: CopyableKlass) {
+    x.fd = FD()
+  }
 
-    do {
-        let x = CopyableKlass()
-        assignCopyableKlass(x)
-    }
-    expectEqual(0, LifetimeTracked.instances)
+  do {
+    let x = CopyableKlass()
+    assignCopyableKlass(x)
+  }
+  expectEqual(0, LifetimeTracked.instances)
 }
 
 var global = FD()
 
 Tests.test("global destroyed once") {
-    do {
-        global = FD()
+  do {
+    global = FD()
+  }
+  expectEqual(0, LifetimeTracked.instances)    
+}
+
+// TODO (rdar://107494072): Move-only types with deinits declared inside
+// functions sometimes lose their deinit function.
+// When that's fixed, FD2 can be moved back inside the test closure below.
+@_moveOnly
+struct FD2 {
+  var field = 5
+  static var count = 0
+  init() { FD2.count += 1 }
+  deinit {
+    FD2.count -= 1
+    print("In deinit!")
+  }
+  func use() {}
+}
+
+Tests.test("deinit not called in init when assigned") {
+  class FDHaver {
+    var fd: FD2
+
+    init() {
+      self.fd = FD2()
     }
-    expectEqual(0, LifetimeTracked.instances)    
+  }
+
+  class FDHaver2 {
+    var fd: FD2
+
+    init() {
+      self.fd = FD2()
+      self.fd = FD2()
+      self.fd = FD2()
+      self.fd.use()
+    }
+  }
+
+  do {
+    let haver = FDHaver2()
+    let _ = haver
+  }
+  expectEqual(0, FD2.count)
 }

test/SILOptimizer/moveonly_class_inits_end_to_end.swift

@@ -0,0 +1,31 @@
+// RUN: %target-swift-frontend -module-name moveonly_class_inits_end_to_end %s -emit-sil -o - | %FileCheck %s
+
+// A test that makes sure end to end in a copyable class containing a
+// non-copyable type, in the init, we only have a single destroy_addr.
+
+@_moveOnly
+public struct MO {
+  var x: Int8 = 0
+  deinit { print("destroyed MO") }
+}
+
+public class MOHaver {
+  var mo: MO
+
+  // CHECK-LABEL: sil hidden @$s028moveonly_class_inits_end_to_D07MOHaverCACycfc : $@convention(method) (@owned MOHaver) -> @owned MOHaver {
+  // CHECK: bb0([[ARG:%.*]] :
+  // CHECK-NEXT: debug_value [[ARG]]
+  // CHECK-NEXT: [[META:%.*]] = metatype
+  // CHECK-NEXT: function_ref MO.init()
+  // CHECK-NEXT: [[FUNC:%.*]] = function_ref @$s028moveonly_class_inits_end_to_D02MOVACycfC :
+  // CHECK-NEXT: [[RESULT:%.*]] = apply [[FUNC]]([[META]])
+  // CHECK-NEXT: [[REF:%.*]] = ref_element_addr [[ARG]]
+  // CHECK-NEXT: [[REF_ACCESS:%.*]] = begin_access [modify] [dynamic] [[REF]]
+  // CHECK-NEXT: store [[RESULT]] to [[REF_ACCESS]]
+  // CHECK-NEXT: end_access [[REF_ACCESS]]
+  // CHECK-NEXT: return [[ARG]]
+  // CHECK-NEXT: } // end sil function '$s028moveonly_class_inits_end_to_D07MOHaverCACycfc'
+  init() {
+    self.mo = MO()
+  }
+}