Merge pull request #71762 from jckarter/borrowing-switch-10

Consume switch subjects under an opaque sub-access.

Author: jckarter

lib/SILGen/SILGenPattern.cpp

@@ -1289,8 +1289,8 @@ void PatternMatchEmission::bindIrrefutablePatterns(const ClauseRow &row,
 
 void PatternMatchEmission::bindIrrefutableBorrows(const ClauseRow &row,
                                                   ArgArray args,
-                                                   bool forIrrefutableRow,
-                                                   bool hasMultipleItems) {
+                                                  bool forIrrefutableRow,
+                                                  bool hasMultipleItems) {
   assert(row.columns() == args.size());
   for (unsigned i = 0, e = args.size(); i != e; ++i) {
     if (!row[i]) // We use null patterns to mean artificial AnyPatterns
@@ -1446,7 +1446,7 @@ void PatternMatchEmission::bindBorrow(Pattern *pattern, VarDecl *var,
     auto access = SGF.B.createBeginAccess(pattern, bindValue.getValue(),
                                           SILAccessKind::Read,
                                           SILAccessEnforcement::Static,
-                                          false, false);
+                                          /*no nested conflict*/ true, false);
     SGF.Cleanups.pushCleanup<EndAccessCleanup>(access);
     bindValue = ManagedValue::forBorrowedAddressRValue(access);
   }
@@ -2797,11 +2797,23 @@ void PatternMatchEmission::emitDestructiveCaseBlocks() {
     // Create a scope to break down the subject value.
     Scope caseScope(SGF, pattern);
 
+    // If the subject value is in memory, enter a deinit access for the memory.
+    // This saves the move-only-checker from trying to analyze the payload
+    // decomposition as a potential partial consume. We always fully consume
+    // the subject on this path.
+    auto origSubject = NoncopyableConsumableValue.forward(SGF);
+    if (origSubject->getType().isAddress()) {
+      origSubject = SGF.B.createBeginAccess(pattern, origSubject,
+                                            SILAccessKind::Deinit,
+                                            SILAccessEnforcement::Static,
+                                            /*no nested conflict*/ true, false);
+      SGF.Cleanups.pushCleanup<EndAccessCleanup>(origSubject);
+    }
+    
     // Clone the original subject's cleanup state so that it will be reliably
     // consumed in this scope, while leaving the original for other case
     // blocks to re-consume.
-    ManagedValue subject = SGF.emitManagedRValueWithCleanup(
-                                     NoncopyableConsumableValue.forward(SGF));
+    ManagedValue subject = SGF.emitManagedRValueWithCleanup(origSubject);
 
     // TODO: handle fallthroughs and multiple cases bindings
     // In those cases we'd need to forward bindings through the shared case
@@ -3428,7 +3440,8 @@ void SILGenFunction::emitSwitchStmt(SwitchStmt *S) {
               // match is not allowed to modify it.
               auto access = B.createBeginAccess(S, subjectMV.getValue(),
                 SILAccessKind::Read,
-                SILAccessEnforcement::Static, false, false);
+                SILAccessEnforcement::Static,
+                /*no nested conflict*/ true, false);
               Cleanups.pushCleanup<EndAccessCleanup>(access);
               subjectMV = ManagedValue::forBorrowedAddressRValue(access);
             }

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

@@ -1978,17 +1978,13 @@ struct GatherUsesVisitor : public TransitiveAddressWalker<GatherUsesVisitor> {
 } // end anonymous namespace
 
 bool GatherUsesVisitor::visitTransitiveUseAsEndPointUse(Operand *op) {
-  // If an access is checked by its own strict mark_unresolved_noncopyable
-  // instruction, then treat the access as an opaque borrowing use from the
-  // outside.
+  // If an access is static and marked as "no nested conflict", we use that
+  // in switch codegen to mark an opaque sub-access that move-only checking
+  // should not look through.
   if (auto ba = dyn_cast<BeginAccessInst>(op->getUser())) {
-    for (auto accessUse : ba->getUses()) {
-      if (auto mark
-         = dyn_cast<MarkUnresolvedNonCopyableValueInst>(accessUse->getUser())) {
-        if (mark->isStrict()) {
-          return true;
-        }
-      }
+    if (ba->getEnforcement() == SILAccessEnforcement::Static
+        && ba->hasNoNestedConflict()) {
+      return true;
     }
   }
   return false;
@@ -2548,20 +2544,29 @@ bool GatherUsesVisitor::visitUse(Operand *op) {
     return true;
   }
 
-  // Treat an opaque read access as a borrow liveness use for the duration
-  // of the access.
   if (auto *access = dyn_cast<BeginAccessInst>(op->getUser())) {
-    assert(access->getAccessKind() == SILAccessKind::Read);
-    LLVM_DEBUG(llvm::dbgs() << "begin_access use\n");
-    
-    auto leafRange = TypeTreeLeafTypeRange::get(op->get(), getRootAddress());
-    if (!leafRange) {
-      LLVM_DEBUG(llvm::dbgs() << "Failed to form leaf type range!\n");
-      return false;
-    }
+    switch (access->getAccessKind()) {
+    // Treat an opaque read access as a borrow liveness use for the duration
+    // of the access.
+    case SILAccessKind::Read: {
+      LLVM_DEBUG(llvm::dbgs() << "begin_access [read]\n");
+      
+      auto leafRange = TypeTreeLeafTypeRange::get(op->get(), getRootAddress());
+      if (!leafRange) {
+        LLVM_DEBUG(llvm::dbgs() << "Failed to form leaf type range!\n");
+        return false;
+      }
 
-    useState.recordLivenessUse(user, *leafRange);
-    return true;
+      useState.recordLivenessUse(user, *leafRange);
+      return true;
+    }
+    // Treat a deinit access as a consume of the entire value.
+    case SILAccessKind::Deinit:
+      llvm_unreachable("should have been handled by `memInstMustConsume`");
+    case SILAccessKind::Init:
+    case SILAccessKind::Modify:
+      llvm_unreachable("should look through these kinds of accesses currently");
+    }
   }
 
   // If we don't fit into any of those categories, just track as a liveness

lib/SILOptimizer/Mandatory/MoveOnlyUtils.cpp

@@ -313,6 +313,8 @@ bool noncopyable::memInstMustConsume(Operand *memOper) {
   case SILInstructionKind::LoadInst:
     return cast<LoadInst>(memInst)->getOwnershipQualifier() ==
            LoadOwnershipQualifier::Take;
+  case SILInstructionKind::BeginAccessInst:
+    return cast<BeginAccessInst>(memInst)->getAccessKind() == SILAccessKind::Deinit;
   }
 }
 

test/SILOptimizer/moveonly_borrowing_switch.swift

@@ -237,8 +237,8 @@ func testOuterAO(consuming bas: consuming AOBas) { // expected-error{{'bas' used
         break
     }
 
-    switch bas {
-    case _borrowing x: // expected-warning{{}} expected-note{{used here}}
+    switch bas { // expected-note{{used here}}
+    case _borrowing x: // expected-warning{{}}
         break
     }
 }

test/SILOptimizer/moveonly_consuming_switch.swift

@@ -0,0 +1,80 @@
+// RUN: %target-swift-frontend -emit-sil -enable-experimental-feature NoncopyableGenerics -enable-experimental-feature BorrowingSwitch -enable-experimental-feature MoveOnlyPartialConsumption -verify %s
+struct MyPointer<Wrapped: ~Copyable>: Copyable {
+    var v: UnsafeMutablePointer<Int>
+
+    static func allocate(capacity: Int) -> Self {
+        fatalError()
+    }
+
+    func initialize(to: consuming Wrapped) {
+    }
+    func deinitialize(count: Int) {
+    }
+    func deallocate() {
+    }
+    func move() -> Wrapped {
+        fatalError()
+    }
+}
+
+struct Box<Wrapped: ~Copyable>: ~Copyable {
+    private let _pointer: MyPointer<Wrapped>
+    
+    init(_ element: consuming Wrapped) {
+        _pointer = .allocate(capacity: 1)
+        print("allocating",_pointer)
+        _pointer.initialize(to: element)
+    }
+        
+    deinit {
+        print("deallocating",_pointer)
+        _pointer.deinitialize(count: 1)
+        _pointer.deallocate()
+    }
+    
+    consuming func move() -> Wrapped {
+        let wrapped = _pointer.move()
+        print("deallocating", _pointer)
+        _pointer.deallocate()
+        discard self
+        return wrapped
+    }
+}
+
+
+
+
+
+
+
+
+
+enum List<Element: ~Copyable>: ~Copyable {
+    case empty
+    case node(Element, Box<List>)
+}
+
+extension List {
+    init() { self = .empty }
+    
+    mutating func push(_ element: consuming Element) {
+        self = .node(element, Box(self))
+    }
+        
+    mutating func pop() -> Element {
+        switch self {
+        case .node(let element, let box):
+            self = box.move()
+            return element
+        case .empty:
+            fatalError()
+        }
+    }
+    
+    var isEmpty: Bool {
+        switch self {
+        case .empty: true
+        case .node: false
+        }
+    }
+}

test/SILOptimizer/moveonly_consuming_switch_2.swift

@@ -0,0 +1,27 @@
+// RUN: %target-swift-frontend -emit-sil -enable-experimental-feature BorrowingSwitch -enable-experimental-feature MoveOnlyPartialConsumption -verify %s
+
+struct Box: ~Copyable {
+    let ptr: UnsafeMutablePointer<Int>
+    deinit { print("butt") }
+}
+
+enum List<Element>: ~Copyable {
+    case end
+    case more(Element, Box)
+}
+
+extension List {
+    init() {
+        self = .end
+    }
+
+    mutating func pop() -> Element {
+        switch consume self {
+        case .more(let element, let box): // expected-warning{{}}
+            self = .end
+            return element
+        case .end: fatalError()
+        }
+    }
+}
+