[move-only] If we have a guaranteed forwarding instruction with only trivial results, treat it as a liveness use.

The reason why we want to do this is that if we treat it as a true forwarding
use, we will visit the uses of the trivial value and treat those as liveness
uses. Since the trivial value is not tied to the lifetime of the underlying
noncopyable value, this can be outside of the lifetime of said value causing a
memory lifetime error. By just treating the guaranteed forwarding instruction
with all trivial values as a liveness use, we avoid this problem.

I added a SIL test, a Swift test, and an Interpreter test that validates this
behavior.

rdar://111497657
(cherry picked from commit 9b59588)

Author: gottesmm

lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureUtils.cpp

@@ -369,16 +369,46 @@ bool Implementation::gatherUses(SILValue value) {
       continue;
     }
 
-    case OperandOwnership::GuaranteedForwarding:
-      // Look through guaranteed forwarding.
+    case OperandOwnership::GuaranteedForwarding: {
+      // Look through guaranteed forwarding if we have at least one non-trivial
+      // value. If we have all non-trivial values, treat this as a liveness use.
+      SmallVector<SILValue, 8> forwardedValues;
+      auto *fn = nextUse->getUser()->getFunction();
       ForwardingOperand(nextUse).visitForwardedValues([&](SILValue value) {
-        for (auto *use : value->getUses()) {
-          useWorklist.push_back(use);
-        }
+        if (value->getType().isTrivial(fn))
+          return true;
+        forwardedValues.push_back(value);
         return true;
       });
-      continue;
 
+      if (forwardedValues.empty()) {
+        auto leafRange =
+          TypeTreeLeafTypeRange::get(nextUse->get(), getRootValue());
+        if (!leafRange) {
+          LLVM_DEBUG(llvm::dbgs() << "        Failed to compute leaf range?!\n");
+          return false;
+        }
+
+        LLVM_DEBUG(llvm::dbgs() << "        Found non lifetime ending use!\n");
+        blocksToUses.insert(nextUse->getParentBlock(),
+                            {nextUse,
+                             {liveness.getNumSubElements(), *leafRange,
+                              false /*is lifetime ending*/}});
+        liveness.updateForUse(nextUse->getUser(), *leafRange,
+                              false /*is lifetime ending*/);
+        instToInterestingOperandIndexMap.insert(nextUse->getUser(), nextUse);
+        continue;
+      }
+
+      // If we had at least one forwarded value that is non-trivial, we need to
+      // visit those uses.
+      while (!forwardedValues.empty()) {
+        for (auto *use : forwardedValues.pop_back_val()->getUses()) {
+          useWorklist.push_back(use);
+        }
+      }
+      continue;
+    }
     case OperandOwnership::Borrow: {
       // Look through borrows.
       if (auto *bbi = dyn_cast<BeginBorrowInst>(nextUse->getUser())) {

test/Interpreter/moveonly_fixedqueue.swift

@@ -0,0 +1,66 @@
+// RUN: %target-run-simple-swift(-Xfrontend -sil-verify-all)
+// RUN: %target-run-simple-swift(-O -Xfrontend -sil-verify-all)
+
+enum FixedSizeQueueError : Error {
+    case outOfSpace
+}
+
+struct FixedSizeQueue<T> : ~Copyable {
+    private var readOffset: Int = 0
+    private var writeOffset: Int = 0
+    private var ptr: UnsafeMutableBufferPointer<T>
+
+    init(size: Int) {
+        ptr = UnsafeMutableBufferPointer.allocate(capacity: size)
+    }
+
+    deinit {
+        ptr.deinitialize().deallocate()
+    }
+
+    mutating func read() -> T? {
+        if readOffset == writeOffset {
+            return nil
+        }
+
+        let x = ptr[readOffset]
+        readOffset = (readOffset + 1) % ptr.count
+        return x
+    }
+
+    mutating func write(_ element: T) throws {
+        if ((writeOffset + 1) % ptr.count) == readOffset {
+            throw FixedSizeQueueError.outOfSpace
+        }
+        ptr[writeOffset] = element
+        writeOffset = (writeOffset + 1) % ptr.count
+    }
+
+    // Drain the queue... returning the value.
+    consuming func drain() -> UnsafeMutableBufferPointer<T> {
+        let buffer = self.ptr
+        discard self
+        return buffer
+    }
+}
+
+func main() throws {
+    var x = FixedSizeQueue<Int>(size: 10)
+    precondition(x.read() == nil)
+    try x.write(0)
+    try x.write(9)
+    try x.write(1)
+    try x.write(3)
+    precondition(x.read() == 0)
+    precondition(x.read() == 9)
+    precondition(x.read() == 1)
+    precondition(x.read() == 3)
+
+    let d = x.drain()
+    precondition(d[0] == 0)
+    precondition(d[1] == 9)
+    precondition(d[2] == 1)
+    precondition(d[3] == 3)
+}
+
+try main()

test/SILOptimizer/moveonly_addresschecker.sil

@@ -677,3 +677,58 @@ bb0:
   %21 = tuple ()
   return %21 : $()
 }
+
+struct FixedSizeQueue : ~Copyable {
+  var ptr: Builtin.Int32
+}
+
+sil @get_fixedsize_queue : $@convention(thin) () -> @owned FixedSizeQueue
+
+// CHECK-LABEL: sil [ossa] @test_return_trivial_type : $@convention(thin) (@owned FixedSizeQueue) -> Builtin.Int32 {
+// CHECK: bb0([[ARG:%.*]] : @owned $
+// CHECK:   [[ALLOC:%.*]] = alloc_stack [lexical] $FixedSizeQueue
+// CHECK:   store [[ARG]] to [init] [[ALLOC]]
+// CHECK:   [[ACCESS:%.*]] = begin_access [modify] [static] [[ALLOC]]
+// CHECK:   [[ALLOC2:%.*]] = alloc_stack $FixedSizeQueue
+// CHECK:   copy_addr [take] [[ACCESS]] to [init] [[ALLOC2]]
+// CHECK:   [[LOADED:%.*]] = load_borrow [[ALLOC2]]
+// CHECK:   end_access [[ACCESS]]
+// CHECK:   [[BORROW:%.*]] = begin_borrow [[LOADED]]
+// CHECK:   [[TRIVIAL_VALUE:%.*]] = struct_extract [[BORROW]]
+// CHECK:   end_borrow [[BORROW]]
+// CHECK:   end_borrow [[LOADED]]
+// CHECK:   destroy_addr [[ALLOC2]]
+// CHECK:   dealloc_stack [[ALLOC2]]
+// CHECK:   [[NEW_VALUE:%.*]] = apply {{%.*}}()
+// CHECK:   [[ACCESS:%.*]] = begin_access [modify] [static] [[ALLOC]]
+// CHECK:   store [[NEW_VALUE]] to [init] [[ACCESS]]
+// CHECK:   end_access [[ACCESS]]
+// CHECK:   destroy_addr [[ALLOC]]
+// CHECK:   dealloc_stack [[ALLOC]]
+// CHECK:   return [[TRIVIAL_VALUE]]
+// CHECK: } // end sil function 'test_return_trivial_type'
+sil [ossa] @test_return_trivial_type : $@convention(thin) (@owned FixedSizeQueue) -> Builtin.Int32 {
+bb0(%0 : @owned $FixedSizeQueue):
+  %1 = alloc_stack [lexical] $FixedSizeQueue, var, name "self", implicit
+  %2 = mark_must_check [consumable_and_assignable] %1 : $*FixedSizeQueue
+  store %0 to [init] %2 : $*FixedSizeQueue
+  %4 = begin_access [modify] [static] %2 : $*FixedSizeQueue
+  %5 = alloc_stack $FixedSizeQueue
+  %6 = mark_must_check [consumable_and_assignable] %5 : $*FixedSizeQueue
+  copy_addr %4 to [init] %6 : $*FixedSizeQueue
+  %8 = load [take] %6 : $*FixedSizeQueue
+  end_access %4 : $*FixedSizeQueue
+  %10 = begin_borrow %8 : $FixedSizeQueue
+  %11 = struct_extract %10 : $FixedSizeQueue, #FixedSizeQueue.ptr
+  end_borrow %10 : $FixedSizeQueue
+  destroy_value %8 : $FixedSizeQueue
+  dealloc_stack %5 : $*FixedSizeQueue
+  %21 = function_ref @get_fixedsize_queue : $@convention(thin) () -> @owned FixedSizeQueue
+  %22 = apply %21() : $@convention(thin) () -> @owned FixedSizeQueue
+  %23 = begin_access [modify] [static] %2 : $*FixedSizeQueue
+  store %22 to [assign] %23 : $*FixedSizeQueue
+  end_access %23 : $*FixedSizeQueue
+  destroy_addr %2 : $*FixedSizeQueue
+  dealloc_stack %1 : $*FixedSizeQueue
+  return %11 : $Builtin.Int32
+}

test/SILOptimizer/moveonly_addresschecker.swift

@@ -0,0 +1,16 @@
+// RUN: %target-swift-emit-sil -O -sil-verify-all -verify -enable-experimental-feature MoveOnlyPartialConsumption -enable-experimental-feature NoImplicitCopy -enable-experimental-feature MoveOnlyClasses %s
+
+// This file contains tests that used to crash due to verifier errors. It must
+// be separate from moveonly_addresschecker_diagnostics since when we fail on
+// the diagnostics in that file, we do not actually run the verifier.
+
+struct TestTrivialReturnValue : ~Copyable {
+    var i: Int = 5
+
+    // We used to error on return buffer.
+    consuming func drain() -> Int {
+        let buffer = (consume self).i
+        self = .init(i: 5)
+        return buffer
+    }
+}

test/SILOptimizer/moveonly_borrow_to_destructure_transform.sil

@@ -147,12 +147,12 @@ bb0(%0 : @owned $MoveOnlyKlass):
 // CHECK:   [[ARG_COPY:%.*]] = copy_value [[ARG]]
 // CHECK:   [[MARKED_ARG:%.*]] = mark_must_check [no_consume_or_assign] [[ARG_COPY]]
 // CHECK:   [[MARKED_ARG_COPY:%.*]] = copy_value [[MARKED_ARG]]
-// CHECK:   [[MISC_USE:%.*]] = function_ref @misc_use : $@convention(thin) () -> ()
-// CHECK:   apply [[MISC_USE]]()
 // CHECK:   [[BORROW:%.*]] = begin_borrow [[MARKED_ARG_COPY]]
 // CHECK:   [[EXT:%.*]] = struct_extract [[BORROW]]
 // CHECK:   end_borrow [[BORROW]]
-// CHECK:   destroy_value [[MARKED_ARG_COPY]]
+// CHECK:   [[MISC_USE:%.*]] = function_ref @misc_use : $@convention(thin) () -> ()
+// CHECK:   apply [[MISC_USE]]()
+// CHECK:   destroy_value [[MARKED_ARG]]
 // CHECK:   return [[EXT]]
 // CHECK: } // end sil function 'test_return_of_extracted_trivial_type'
 sil [ossa] @test_return_of_extracted_trivial_type : $@convention(thin) (@guaranteed SingleIntContainingStruct) -> Builtin.Int32 {
@@ -862,9 +862,9 @@ bbCont:
 // CHECK: [[BB_E_SECOND]]([[BBARG:%.*]] : @owned
 // CHECK:   [[BBARG_BORROW:%.*]] = begin_borrow [[BBARG]]
 // CHECK:   [[BBARG_BORROW_EXT:%.*]] = struct_extract [[BBARG_BORROW]]
-// CHECK:   apply {{%.*}}([[BBARG_BORROW_EXT]])
 // CHECK:   end_borrow [[BBARG_BORROW]]
 // CHECK:   destroy_value [[BBARG]]
+// CHECK:   apply {{%.*}}([[BBARG_BORROW_EXT]])
 // CHECK:   br [[BB_CONT]]
 //
 // CHECK: [[BB_E_THIRD]]([[BBARG:%.*]] : @owned
@@ -955,9 +955,9 @@ bbCont:
 // CHECK: [[BB_E_SECOND]]([[BBARG:%.*]] : @owned
 // CHECK:   [[BBARG_BORROW:%.*]] = begin_borrow [[BBARG]]
 // CHECK:   [[BBARG_BORROW_EXT:%.*]] = struct_extract [[BBARG_BORROW]]
-// CHECK:   apply {{%.*}}([[BBARG_BORROW_EXT]])
 // CHECK:   end_borrow [[BBARG_BORROW]]
 // CHECK:   destroy_value [[BBARG]]
+// CHECK:   apply {{%.*}}([[BBARG_BORROW_EXT]])
 // CHECK:   br [[BB_CONT]]
 //
 // CHECK: [[BB_E_THIRD]]([[BBARG:%.*]] : @owned
@@ -1076,4 +1076,4 @@ bb2:
   end_borrow %3 : $KlassPair
   destroy_value %2 : $KlassPair
   unwind
-}
+}