SIL: some improvements/fixes around assign_by_wrapper

* Refactoring: replace "Destination" and the ownership qualifier by a single "Mode".  This represents much better the mode how the instruction is to be lowered. NFC
* Make assign_by_wrapper printable and parseable.
* Fix lowering of the assign modes for indirect results of the init-closure: The indirect result was initialized and not assigned to. The fix is to insert a destroy_addr before calling the init closure. This fixes a memory lifetime error and/or a memory leak. Found by inspection.
* Fix an iterator-invalidation crash in RawSILInstLowering
* Add tests for lowering assign_by_wrapper.

Author: eeckstein

docs/SIL.rst

@@ -3642,7 +3642,9 @@ assign_by_wrapper
 ``````````````````
 ::
 
-  sil-instruction ::= 'assign_by_wrapper' sil-operand 'to' sil-operand ',' 'init' sil-operand ',' 'set' sil-operand
+  sil-instruction ::= 'assign_by_wrapper' sil-operand 'to' mode? sil-operand ',' 'init' sil-operand ',' 'set' sil-operand
+
+  mode ::= '[initialization]' | '[assign]' | '[assign_wrapped_value]'
 
   assign_by_wrapper %0 : $S to %1 : $*T, init %2 : $F, set %3 : $G
   // $S can be a value or address type
@@ -3653,13 +3655,22 @@ assign_by_wrapper
 Similar to the `assign`_ instruction, but the assignment is done via a
 delegate.
 
-In case of an initialization, the function ``%2`` is called with ``%0`` as
-argument. The result is stored to ``%1``. In case ``%2`` is an address type,
-it is simply passed as a first out-argument to ``%2``.
+Initially the instruction is created with no mode. Once the mode is decided
+(by the definitive initialization pass), the instruction is lowered as follows:
+
+If the mode is ``initialization``, the function ``%2`` is called with ``%0`` as
+argument. The result is stored to ``%1``. In case of an address type, ``%1`` is
+simply passed as a first out-argument to ``%2``.
 
-In case of a re-assignment, the function ``%3`` is called with ``%0`` as
-argument. As ``%3`` is a setter (e.g. for the property in the containing
-nominal type), the destination address ``%1`` is not used in this case.
+The ``assign`` mode works similar to ``initialization``, except that the
+destination is "assigned" rather than "initialized". This means that the
+existing value in the destination is destroyed before the new value is
+stored.
+
+If the mode is ``assign_wrapped_value``, the function ``%3`` is called with
+``%0`` as argument. As ``%3`` is a setter (e.g. for the property in the
+containing nominal type), the destination address ``%1`` is not used in this
+case.
 
 This instruction is only valid in Raw SIL and is rewritten as appropriate
 by the definitive initialization pass.

include/swift/SIL/SILBuilder.h

@@ -877,10 +877,10 @@ class SILBuilder {
                                                SILValue Src, SILValue Dest,
                                                SILValue Initializer,
                                                SILValue Setter,
-                                          AssignOwnershipQualifier Qualifier) {
+                                               AssignByWrapperInst::Mode mode) {
     return insert(new (getModule())
                   AssignByWrapperInst(getSILDebugLocation(Loc), Src, Dest,
-                                       Initializer, Setter, Qualifier));
+                                       Initializer, Setter, mode));
   }
 
   StoreBorrowInst *createStoreBorrow(SILLocation Loc, SILValue Src,

include/swift/SIL/SILCloner.h

@@ -1243,7 +1243,7 @@ void SILCloner<ImplClass>::visitAssignByWrapperInst(AssignByWrapperInst *Inst) {
                                       getOpValue(Inst->getDest()),
                                       getOpValue(Inst->getInitializer()),
                                       getOpValue(Inst->getSetter()),
-                                      Inst->getOwnershipQualifier()));
+                                      Inst->getMode()));
 }
 
 template<typename ImplClass>

include/swift/SIL/SILInstruction.h

@@ -4370,39 +4370,38 @@ class AssignByWrapperInst
   friend SILBuilder;
 
 public:
-  /// The assignment destination for the property wrapper
-  enum class Destination {
-    BackingWrapper,
-    WrappedValue,
+  enum Mode {
+    /// The mode is not decided yet (by DefiniteInitialization).
+    Unknown,
+    
+    /// The initializer is called with Src as argument. The result is stored to
+    /// Dest.
+    Initialization,
+    
+    // Like ``Initialization``, except that the destination is "assigned" rather
+    // than "initialized". This means that the existing value in the destination
+    // is destroyed before the new value is stored.
+    Assign,
+    
+    /// The setter is called with Src as argument. The Dest is not used in this
+    /// case.
+    AssignWrappedValue
   };
 
 private:
-  Destination AssignDest = Destination::WrappedValue;
-
   AssignByWrapperInst(SILDebugLocation DebugLoc, SILValue Src, SILValue Dest,
-                       SILValue Initializer, SILValue Setter,
-                       AssignOwnershipQualifier Qualifier =
-                         AssignOwnershipQualifier::Unknown);
+                       SILValue Initializer, SILValue Setter, Mode mode);
 
 public:
-
   SILValue getInitializer() { return Operands[2].get(); }
   SILValue getSetter() { return  Operands[3].get(); }
 
-  AssignOwnershipQualifier getOwnershipQualifier() const {
-    return AssignOwnershipQualifier(
-      SILNode::Bits.AssignByWrapperInst.OwnershipQualifier);
+  Mode getMode() const {
+    return Mode(SILNode::Bits.AssignByWrapperInst.Mode);
   }
 
-  Destination getAssignDestination() const { return AssignDest; }
-
-  void setAssignInfo(AssignOwnershipQualifier qualifier, Destination dest) {
-    assert(qualifier == AssignOwnershipQualifier::Init && dest == Destination::BackingWrapper ||
-           qualifier == AssignOwnershipQualifier::Reassign && dest == Destination::BackingWrapper ||
-           qualifier == AssignOwnershipQualifier::Reassign && dest == Destination::WrappedValue);
-
-    SILNode::Bits.AssignByWrapperInst.OwnershipQualifier = unsigned(qualifier);
-    AssignDest = dest;
+  void setMode(Mode mode) {
+    SILNode::Bits.AssignByWrapperInst.Mode = unsigned(mode);
   }
 };
 

include/swift/SIL/SILNode.h

@@ -120,6 +120,7 @@ class alignas(8) SILNode {
   enum { NumStoreOwnershipQualifierBits = 2 };
   enum { NumLoadOwnershipQualifierBits = 2 };
   enum { NumAssignOwnershipQualifierBits = 2 };
+  enum { NumAssignByWrapperModeBits = 2 };
   enum { NumSILAccessKindBits = 2 };
   enum { NumSILAccessEnforcementBits = 2 };
 
@@ -311,8 +312,8 @@ class alignas(8) SILNode {
     OwnershipQualifier : NumAssignOwnershipQualifierBits
   );
   SWIFT_INLINE_BITFIELD(AssignByWrapperInst, NonValueInstruction,
-                        NumAssignOwnershipQualifierBits,
-    OwnershipQualifier : NumAssignOwnershipQualifierBits
+                        NumAssignByWrapperModeBits,
+    Mode : NumAssignByWrapperModeBits
   );
 
   SWIFT_INLINE_BITFIELD(UncheckedOwnershipConversionInst,SingleValueInstruction,

lib/SIL/IR/SILInstructions.cpp

@@ -1080,11 +1080,10 @@ AssignByWrapperInst::AssignByWrapperInst(SILDebugLocation Loc,
                                            SILValue Src, SILValue Dest,
                                            SILValue Initializer,
                                            SILValue Setter,
-                                          AssignOwnershipQualifier Qualifier) :
+                                           AssignByWrapperInst::Mode mode) :
     AssignInstBase(Loc, Src, Dest, Initializer, Setter) {
   assert(Initializer->getType().is<SILFunctionType>());
-  SILNode::Bits.AssignByWrapperInst.OwnershipQualifier =
-      unsigned(Qualifier);
+  SILNode::Bits.AssignByWrapperInst.Mode = unsigned(mode);
 }
 
 MarkFunctionEscapeInst *

lib/SIL/IR/SILPrinter.cpp

@@ -1453,7 +1453,19 @@ class SILPrinter : public SILInstructionVisitor<SILPrinter> {
 
   void visitAssignByWrapperInst(AssignByWrapperInst *AI) {
     *this << getIDAndType(AI->getSrc()) << " to ";
-    printAssignOwnershipQualifier(AI->getOwnershipQualifier());
+    switch (AI->getMode()) {
+    case AssignByWrapperInst::Unknown:
+      break;
+    case AssignByWrapperInst::Initialization:
+      *this << "[initialization] ";
+      break;
+    case AssignByWrapperInst::Assign:
+      *this << "[assign] ";
+      break;
+    case AssignByWrapperInst::AssignWrappedValue:
+      *this << "[assign_wrapped_value] ";
+      break;
+    }
     *this << getIDAndType(AI->getDest())
           << ", init " << getIDAndType(AI->getInitializer())
           << ", set " << getIDAndType(AI->getSetter());

lib/SIL/Parser/ParseSIL.cpp

@@ -1981,6 +1981,32 @@ static bool parseAssignOwnershipQualifier(AssignOwnershipQualifier &Result,
   return false;
 }
 
+static bool parseAssignByWrapperMode(AssignByWrapperInst::Mode &Result,
+                                          SILParser &P) {
+  StringRef Str;
+  // If we do not parse '[' ... ']', we have unknown. Set value and return.
+  if (!parseSILOptional(Str, P)) {
+    Result = AssignByWrapperInst::Unknown;
+    return false;
+  }
+
+  // Then try to parse one of our other initialization kinds. We do not support
+  // parsing unknown here so we use that as our fail value.
+  auto Tmp = llvm::StringSwitch<AssignByWrapperInst::Mode>(Str)
+        .Case("initialization", AssignByWrapperInst::Initialization)
+        .Case("assign", AssignByWrapperInst::Assign)
+        .Case("assign_wrapped_value", AssignByWrapperInst::AssignWrappedValue)
+        .Default(AssignByWrapperInst::Unknown);
+
+  // Thus return true (following the conventions in this file) if we fail.
+  if (Tmp == AssignByWrapperInst::Unknown)
+    return true;
+
+  // Otherwise, assign Result and return false.
+  Result = Tmp;
+  return false;
+}
+
 // Parse a list of integer indices, prefaced with the given string label.
 // Returns true on error.
 static bool parseIndexList(Parser &P, StringRef label,
@@ -3659,9 +3685,9 @@ bool SILParser::parseSpecificSILInstruction(SILBuilder &B,
   case SILInstructionKind::AssignByWrapperInst: {
     SILValue Src, DestAddr, InitFn, SetFn;
     SourceLoc DestLoc;
-    AssignOwnershipQualifier AssignQualifier;
+    AssignByWrapperInst::Mode mode;
     if (parseTypedValueRef(Src, B) || parseVerbatim("to") ||
-        parseAssignOwnershipQualifier(AssignQualifier, *this) ||
+        parseAssignByWrapperMode(mode, *this) ||
         parseTypedValueRef(DestAddr, DestLoc, B) ||
         P.parseToken(tok::comma, diag::expected_tok_in_sil_instr, ",") ||
         parseVerbatim("init") || parseTypedValueRef(InitFn, B) ||
@@ -3677,7 +3703,7 @@ bool SILParser::parseSpecificSILInstruction(SILBuilder &B,
     }
 
     ResultVal = B.createAssignByWrapper(InstLoc, Src, DestAddr, InitFn, SetFn,
-                                        AssignQualifier);
+                                        mode);
     break;
   }
 

lib/SILGen/SILGenLValue.cpp

@@ -1549,7 +1549,7 @@ namespace {
 
         SGF.B.createAssignByWrapper(loc, Mval.forward(SGF), proj.forward(SGF),
                                      initFn.getValue(), setterFn.getValue(),
-                                     AssignOwnershipQualifier::Unknown);
+                                     AssignByWrapperInst::Unknown);
 
         return;
       }

lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp

@@ -2012,16 +2012,13 @@ void LifetimeChecker::updateInstructionForInitState(unsigned UseID) {
 
     switch (Use.Kind) {
     case DIUseKind::Initialization:
-      AI->setAssignInfo(AssignOwnershipQualifier::Init,
-                        AssignByWrapperInst::Destination::BackingWrapper);
+      AI->setMode(AssignByWrapperInst::Initialization);
       break;
     case DIUseKind::Assign:
-      AI->setAssignInfo(AssignOwnershipQualifier::Reassign,
-                        AssignByWrapperInst::Destination::BackingWrapper);
+      AI->setMode(AssignByWrapperInst::Assign);
       break;
     case DIUseKind::AssignWrappedValue:
-      AI->setAssignInfo(AssignOwnershipQualifier::Reassign,
-                        AssignByWrapperInst::Destination::WrappedValue);
+      AI->setMode(AssignByWrapperInst::AssignWrappedValue);
       break;
     default:
       llvm_unreachable("Wrong use kind for assign_by_wrapper");

lib/SILOptimizer/Mandatory/RawSILInstLowering.cpp

@@ -167,44 +167,52 @@ static void getAssignByWrapperArgs(SmallVectorImpl<SILValue> &args,
 
 static void lowerAssignByWrapperInstruction(SILBuilderWithScope &b,
                                              AssignByWrapperInst *inst,
-                            SmallVectorImpl<BeginAccessInst *> &accessMarkers) {
-  LLVM_DEBUG(llvm::dbgs() << "  *** Lowering [isInit="
-             << unsigned(inst->getOwnershipQualifier())
-             << "]: " << *inst << "\n");
+                                        SmallSetVector<SILValue, 8> &toDelete) {
+  LLVM_DEBUG(llvm::dbgs() << "  *** Lowering " << *inst << "\n");
 
   ++numAssignRewritten;
 
   SILValue src = inst->getSrc();
   SILValue dest = inst->getDest();
   SILLocation loc = inst->getLoc();
   SILBuilderWithScope forCleanup(std::next(inst->getIterator()));
-  SingleValueInstruction *closureToDelete = nullptr;
+  
+  switch (inst->getMode()) {
+    case AssignByWrapperInst::Unknown:
+      llvm_unreachable("assign_by_wrapper must have a valid mode");
 
-  switch (inst->getAssignDestination()) {
-    case AssignByWrapperInst::Destination::BackingWrapper: {
+    case AssignByWrapperInst::Initialization:
+    case AssignByWrapperInst::Assign: {
       SILValue initFn = inst->getInitializer();
       CanSILFunctionType fTy = initFn->getType().castTo<SILFunctionType>();
       SILFunctionConventions convention(fTy, inst->getModule());
       SmallVector<SILValue, 4> args;
       if (convention.hasIndirectSILResults()) {
+        if (inst->getMode() == AssignByWrapperInst::Assign)
+          b.createDestroyAddr(loc, dest);
+
         args.push_back(dest);
         getAssignByWrapperArgs(args, src, convention, b, forCleanup);
         b.createApply(loc, initFn, SubstitutionMap(), args);
       } else {
         getAssignByWrapperArgs(args, src, convention, b, forCleanup);
         SILValue wrappedSrc = b.createApply(loc, initFn, SubstitutionMap(),
                                             args);
-        if (inst->getOwnershipQualifier() == AssignOwnershipQualifier::Init ||
+        if (inst->getMode() == AssignByWrapperInst::Initialization ||
             inst->getDest()->getType().isTrivial(*inst->getFunction())) {
-          b.createTrivialStoreOr(loc, wrappedSrc, dest, StoreOwnershipQualifier::Init);
+          b.createTrivialStoreOr(loc, wrappedSrc, dest,
+                                 StoreOwnershipQualifier::Init);
         } else {
           b.createStore(loc, wrappedSrc, dest, StoreOwnershipQualifier::Assign);
         }
       }
-      closureToDelete = dyn_cast<SingleValueInstruction>(inst->getSetter());
+      // The unused partial_apply violates memory lifetime rules in case "self"
+      // is an inout. Therefore we cannot keep it as a dead closure to be
+      // cleaned up later. We have to delete it in this pass.
+      toDelete.insert(inst->getSetter());
       break;
     }
-    case AssignByWrapperInst::Destination::WrappedValue: {
+    case AssignByWrapperInst::AssignWrappedValue: {
       SILValue setterFn = inst->getSetter();
       CanSILFunctionType fTy = setterFn->getType().castTo<SILFunctionType>();
       SILFunctionConventions convention(fTy, inst->getModule());
@@ -217,15 +225,15 @@ static void lowerAssignByWrapperInstruction(SILBuilderWithScope &b,
       // marker. This is important, because also the setter function contains
       // access marker. In case those markers are dynamic it would cause a
       // nested access violation.
-      if (auto *BA = dyn_cast<BeginAccessInst>(dest))
-        accessMarkers.push_back(BA);
-      closureToDelete = dyn_cast<SingleValueInstruction>(inst->getInitializer());
+      if (isa<BeginAccessInst>(dest))
+        toDelete.insert(dest);
+        
+      // Again, we have to delete the unused dead closure.
+      toDelete.insert(inst->getInitializer());
       break;
     }
   }
   inst->eraseFromParent();
-  if (closureToDelete)
-    tryDeleteDeadClosure(closureToDelete);
 }
 
 static void deleteDeadAccessMarker(BeginAccessInst *BA) {
@@ -250,7 +258,7 @@ static bool lowerRawSILOperations(SILFunction &fn) {
   bool changed = false;
 
   for (auto &bb : fn) {
-    SmallVector<BeginAccessInst *, 8> accessMarkers;
+    SmallSetVector<SILValue, 8> toDelete;
 
     auto i = bb.begin(), e = bb.end();
     while (i != e) {
@@ -280,7 +288,7 @@ static bool lowerRawSILOperations(SILFunction &fn) {
 
       if (auto *ai = dyn_cast<AssignByWrapperInst>(inst)) {
         SILBuilderWithScope b(ai);
-        lowerAssignByWrapperInstruction(b, ai, accessMarkers);
+        lowerAssignByWrapperInstruction(b, ai, toDelete);
         changed = true;
         continue;
       }
@@ -300,8 +308,12 @@ static bool lowerRawSILOperations(SILFunction &fn) {
         continue;
       }
     }
-    for (BeginAccessInst *BA : accessMarkers) {
-      deleteDeadAccessMarker(BA);
+    for (SILValue deadVal : toDelete) {
+      if (auto *beginAccess = dyn_cast<BeginAccessInst>(deadVal)) {
+        deleteDeadAccessMarker(beginAccess);
+      } else if (auto *svi = dyn_cast<SingleValueInstruction>(deadVal)) {
+        tryDeleteDeadClosure(svi);
+      }
     }
   }
   return changed;