Fix MemoryLifetimeVerifier and SIL verifier to handle trivial deinitialization

include/swift/SIL/MemoryLocations.h

@@ -251,6 +251,13 @@ class MemoryLocations {
     if (auto *loc = getLocation(addr))
       bits.reset(loc->subLocations);
   }
+
+  // Nontrivial values are never deinitialized. For forward dataflow, use
+  // clearNonTrivialBits for operations that deinitialize without deallocating.
+  void clearNonTrivialBits(Bits &bits, SILValue addr) {
+    if (auto *loc = getLocation(addr))
+      bits.reset(loc->subLocations & getNonTrivialLocations());
+  }
 
   void genBits(Bits &genSet, Bits &killSet, SILValue addr) const {
     if (auto *loc = getLocation(addr)) {
@@ -266,6 +273,16 @@ class MemoryLocations {
     }
   }
 
+  // Nontrivial values are never deinitialized. For forward dataflow, use
+  // killNonTrivialBits for operations that deinitialize without deallocating.
+  void killNonTrivialBits(Bits &genSet, Bits &killSet, SILValue addr) {
+    if (auto *loc = getLocation(addr)) {
+      Bits killLocations = loc->subLocations & getNonTrivialLocations();
+      killSet |= killLocations;
+      genSet.reset(killLocations);
+    }
+  }
+
   /// Analyzes all locations in a function.
   ///
   /// Single-block locations are not analyzed, but added to singleBlockLocations.

lib/SIL/Verifier/MemoryLifetimeVerifier.cpp

@@ -131,6 +131,10 @@ class MemoryLifetimeVerifier {
     locations.killBits(blockState.genSet, blockState.killSet, addr);
   }
 
+  void killNonTrivialBits(BitDataflow::BlockState &blockState, SILValue addr) {
+    locations.killNonTrivialBits(blockState.genSet, blockState.killSet, addr);
+  }
+
 public:
   MemoryLifetimeVerifier(SILFunction *function) :
     function(function), locations(/*handleNonTrivialProjections*/ true,
@@ -335,7 +339,7 @@ void MemoryLifetimeVerifier::initDataflowInBlock(SILBasicBlock *block,
         auto *LI = cast<LoadInst>(&I);
         switch (LI->getOwnershipQualifier()) {
           case LoadOwnershipQualifier::Take:
-            killBits(state, LI->getOperand());
+            killNonTrivialBits(state, LI->getOperand());
             break;
           default:
             break;
@@ -354,7 +358,7 @@ void MemoryLifetimeVerifier::initDataflowInBlock(SILBasicBlock *block,
       case SILInstructionKind::CopyAddrInst: {
         auto *CAI = cast<CopyAddrInst>(&I);
         if (CAI->isTakeOfSrc())
-          killBits(state, CAI->getSrc());
+          killNonTrivialBits(state, CAI->getSrc());
         genBits(state, CAI->getDest());
         break;
       }
@@ -385,14 +389,16 @@ void MemoryLifetimeVerifier::initDataflowInBlock(SILBasicBlock *block,
         break;
       }
       case SILInstructionKind::DestroyAddrInst:
+        killNonTrivialBits(state, I.getOperand(0));
+        break;
       case SILInstructionKind::DeallocStackInst:
         killBits(state, I.getOperand(0));
         break;
       case SILInstructionKind::UncheckedRefCastAddrInst:
       case SILInstructionKind::UnconditionalCheckedCastAddrInst: {
         SILValue src = I.getOperand(CopyLikeInstruction::Src);
         SILValue dest = I.getOperand(CopyLikeInstruction::Dest);
-        killBits(state, src);
+        killNonTrivialBits(state, src);
         genBits(state, dest);
         break;
       }
@@ -478,11 +484,11 @@ void MemoryLifetimeVerifier::setBitsOfPredecessor(Bits &getSet, Bits &killSet,
   } else if (auto *castInst = dyn_cast<CheckedCastAddrBranchInst>(term)) {
     switch (castInst->getConsumptionKind()) {
     case CastConsumptionKind::TakeAlways:
-      locations.killBits(getSet, killSet, castInst->getSrc());
+      locations.killNonTrivialBits(getSet, killSet, castInst->getSrc());
       break;
     case CastConsumptionKind::TakeOnSuccess:
       if (castInst->getSuccessBB() == block)
-        locations.killBits(getSet, killSet, castInst->getSrc());
+        locations.killNonTrivialBits(getSet, killSet, castInst->getSrc());
       break;
     case CastConsumptionKind::CopyOnSuccess:
       break;
@@ -499,7 +505,7 @@ void MemoryLifetimeVerifier::setFuncOperandBits(BlockState &state, Operand &op,
                                         bool isTryApply) {
   switch (convention) {
     case SILArgumentConvention::Indirect_In:
-      killBits(state, op.get());
+      killNonTrivialBits(state, op.get());
       break;
     case SILArgumentConvention::Indirect_Out:
       // try_apply is special, because an @out result is only initialized
@@ -600,7 +606,7 @@ void MemoryLifetimeVerifier::checkBlock(SILBasicBlock *block, Bits &bits) {
         requireBitsSet(bits, LI->getOperand(), &I);
         switch (LI->getOwnershipQualifier()) {
           case LoadOwnershipQualifier::Take:
-            locations.clearBits(bits, LI->getOperand());
+            locations.clearNonTrivialBits(bits, LI->getOperand());
             requireNoStoreBorrowLocation(LI->getOperand(), &I);
             break;
           case LoadOwnershipQualifier::Copy:
@@ -639,7 +645,7 @@ void MemoryLifetimeVerifier::checkBlock(SILBasicBlock *block, Bits &bits) {
         auto *CAI = cast<CopyAddrInst>(&I);
         requireBitsSet(bits, CAI->getSrc(), &I);
         if (CAI->isTakeOfSrc()) {
-          locations.clearBits(bits, CAI->getSrc());
+          locations.clearNonTrivialBits(bits, CAI->getSrc());
           requireNoStoreBorrowLocation(CAI->getSrc(), &I);
         }
         if (CAI->isInitializationOfDest()) {
@@ -702,7 +708,7 @@ void MemoryLifetimeVerifier::checkBlock(SILBasicBlock *block, Bits &bits) {
       case SILInstructionKind::DestroyAddrInst: {
         SILValue opVal = cast<DestroyAddrInst>(&I)->getOperand();
         requireBitsSet(bits | ~nonTrivialLocations, opVal, &I);
-        locations.clearBits(bits, opVal);
+        locations.clearNonTrivialBits(bits, opVal);
         requireNoStoreBorrowLocation(opVal, &I);
         break;
       }
@@ -721,7 +727,7 @@ void MemoryLifetimeVerifier::checkBlock(SILBasicBlock *block, Bits &bits) {
         SILValue src = I.getOperand(CopyLikeInstruction::Src);
         SILValue dest = I.getOperand(CopyLikeInstruction::Dest);
         requireBitsSet(bits, src, &I);
-        locations.clearBits(bits, src);
+        locations.clearNonTrivialBits(bits, src);
         requireBitsClear(bits & nonTrivialLocations, dest, &I);
         locations.setBits(bits, dest);
         requireNoStoreBorrowLocation(dest, &I);
@@ -816,7 +822,7 @@ void MemoryLifetimeVerifier::checkFuncArgument(Bits &bits, Operand &argumentOp,
   switch (argumentConvention) {
     case SILArgumentConvention::Indirect_In:
       requireBitsSet(bits, argumentOp.get(), applyInst);
-      locations.clearBits(bits, argumentOp.get());
+      locations.clearNonTrivialBits(bits, argumentOp.get());
       break;
     case SILArgumentConvention::Indirect_Out:
       requireBitsClear(bits & locations.getNonTrivialLocations(),

lib/SIL/Verifier/SILVerifier.cpp

@@ -450,10 +450,19 @@ void verifyKeyPathComponent(SILModule &M,
 /// Check if according to the SIL language model this memory /must only/ be used
 /// immutably. Today this is only applied to in_guaranteed arguments and
 /// open_existential_addr. We should expand it as needed.
+///
+/// Consuming a trivial type has no effect, so we return false in that
+/// case. SILGen avoids generating copies and destroys for trivial types when
+/// ownership would otherwise require it.
 struct ImmutableAddressUseVerifier {
   SmallVector<Operand *, 32> worklist;
 
-  bool isConsumingOrMutatingArgumentConvention(SILArgumentConvention conv) {
+  bool isTrivial(Operand *use) {
+    SILValue value = use->get();
+    return value->getType().isTrivial(value->getFunction());
+  }
+
+  bool isConsumingOrMutatingArgumentConvention(SILArgumentConvention conv, bool isTrivial) {
     switch (conv) {
     case SILArgumentConvention::Indirect_In_Guaranteed:
     case SILArgumentConvention::Pack_Guaranteed:
@@ -471,17 +480,19 @@ struct ImmutableAddressUseVerifier {
 
     case SILArgumentConvention::Pack_Out:
     case SILArgumentConvention::Pack_Owned:
-    case SILArgumentConvention::Pack_Inout:
     case SILArgumentConvention::Indirect_Out:
     case SILArgumentConvention::Indirect_In:
+      return !isTrivial;
+
+    case SILArgumentConvention::Pack_Inout:
     case SILArgumentConvention::Indirect_Inout:
       return true;
 
     case SILArgumentConvention::Direct_Unowned:
     case SILArgumentConvention::Direct_Guaranteed:
     case SILArgumentConvention::Direct_Owned:
       assert(conv.isIndirectConvention() && "Expect an indirect convention");
-      return true; // return something "conservative".
+      return !isTrivial; // return something "conservative".
     }
     llvm_unreachable("covered switch isn't covered?!");
   }
@@ -490,14 +501,14 @@ struct ImmutableAddressUseVerifier {
     ApplySite apply(use->getUser());
     assert(apply && "Not an apply instruction kind");
     auto conv = apply.getArgumentConvention(*use);
-    return isConsumingOrMutatingArgumentConvention(conv);
+    return isConsumingOrMutatingArgumentConvention(conv, isTrivial(use));
   }
 
   bool isConsumingOrMutatingYieldUse(Operand *use) {
     // For now, just say that it is non-consuming for now.
     auto *yield = cast<YieldInst>(use->getUser());
     auto conv = yield->getArgumentConventionForOperand(*use);
-    return isConsumingOrMutatingArgumentConvention(conv);
+    return isConsumingOrMutatingArgumentConvention(conv, isTrivial(use));
   }
 
   // A "copy_addr %src [take] to *" is consuming on "%src".
@@ -507,7 +518,7 @@ struct ImmutableAddressUseVerifier {
     if (copyAddr->getDest() == use->get())
       return true;
     if (copyAddr->getSrc() == use->get() && copyAddr->isTakeOfSrc() == IsTake)
-      return true;
+      return !isTrivial(use);
     return false;
   }
 
@@ -516,7 +527,7 @@ struct ImmutableAddressUseVerifier {
     if (copyAddr->getDest() == use->get())
       return true;
     if (copyAddr->getSrc() == use->get() && copyAddr->isTakeOfSrc() == IsTake)
-      return true;
+      return !isTrivial(use);
     return false;
   }
 

test/SIL/memory_lifetime.sil

@@ -726,3 +726,34 @@ entry:
   %retval = tuple ()
   return %retval : $()
 }
+
+// Reloading a trivial value is ok after the aggregate is deinitialized.
+sil [ossa] @test_mixed : $@convention(thin) (@in Mixed, Int) -> Int {
+bb0(%0 : $*Mixed, %1 : $Int):
+  %2 = struct_element_addr %0 : $*Mixed, #Mixed.i
+  store %1 to [trivial] %2 : $*Int
+  destroy_addr %0 : $*Mixed
+  %3 = load [trivial] %2 : $*Int
+  return %3 : $Int
+}
+
+// rdar://108001491 (SIL verification failed: Found mutating or
+// consuming use of an in_guaranteed parameter?!:
+// !ImmutableAddressUseVerifier().isMutatingOrConsuming(fArg))
+//
+// Allow an argument of guaranteed trivial type to be consumed.
+
+// Passed by address, but does not need to be resilient for this SIL test.
+struct ResilientValue {
+  var value: Builtin.Int32
+}
+
+sil @takeResilientValue : $@convention(thin) (@in ResilientValue) -> ()
+
+sil [thunk] [ossa] @testImutableAddressArgument : $@convention(thin) (@in_guaranteed ResilientValue) -> () {
+bb0(%0 : $*ResilientValue):
+  %1 = function_ref @takeResilientValue : $@convention(thin) (@in ResilientValue) -> ()
+  %2 = apply %1(%0) : $@convention(thin) (@in ResilientValue) -> ()
+  %v = tuple ()
+  return %v : $()
+}

test/SIL/memory_lifetime_failures.sil

@@ -142,13 +142,14 @@ bb0(%0 : @owned $T):
 }
 
 // CHECK: SIL memory lifetime failure in @test_mixed: memory is not initialized, but should be
-sil [ossa] @test_mixed : $@convention(thin) (@in Mixed, Int) -> Int {
+sil [ossa] @test_mixed : $@convention(thin) (@in Mixed, Int) -> @owned T {
 bb0(%0 : $*Mixed, %1 : $Int):
   %2 = struct_element_addr %0 : $*Mixed, #Mixed.i
   store %1 to [trivial] %2 : $*Int
   destroy_addr %0 : $*Mixed
-  %3 = load [trivial] %2 : $*Int
-  return %3 : $Int
+  %5 = struct_element_addr %0 : $*Mixed, #Mixed.t
+  %6 = load [copy] %5 : $*T
+  return %6 : $T
 }
 
 // CHECK: SIL memory lifetime failure in @test_missing_store_to_trivial: memory is not initialized, but should be

test/SILGen/enum_resilience.swift

@@ -206,3 +206,24 @@ public func resilientIfCase(_ e: MyResilientEnum) -> Bool {
   case .loki: ()
   }
 }
+
+
+// -----------------------------------------------------------------------------
+// rdar://108001491 (SIL verification failed: Found mutating or
+// consuming use of an in_guaranteed parameter?!:
+// !ImmutableAddressUseVerifier().isMutatingOrConsuming(fArg))
+//
+// The entityReference thunk requires @in_guaranteed ownership, but
+// the enum initializer takes its argument @in.
+
+public struct EntityIdentifier {
+  public let value: Swift.UInt64
+}
+
+public protocol ObjectProtocol {
+  static func entityReference(_ id: EntityIdentifier) -> Self
+}
+
+public enum Object : ObjectProtocol {
+  case entityReference(EntityIdentifier)
+}