SIL: Enclose switch subjects in a new begin_borrow [fixed] variant.

jckarter · jckarter · commit d75a62ce6481 · 2024-02-21T20:41:20.000-08:00
We want to preserve the borrow scope during switch dispatch so that move-only
checking doesn't try to analyze destructures or consumes out of it. SILGen
should mark anywhere that's a potential possibility with its own marker so that
it gets borrow checked independently.
diff --git a/include/swift/SIL/SILBuilder.h b/include/swift/SIL/SILBuilder.h
@@ -818,12 +818,13 @@ class SILBuilder {
   BeginBorrowInst *createBeginBorrow(SILLocation Loc, SILValue LV,
                                      bool isLexical = false,
                                      bool hasPointerEscape = false,
-                                     bool fromVarDecl = false) {
+                                     bool fromVarDecl = false,
+                                     bool fixed = false) {
     assert(getFunction().hasOwnership());
     assert(!LV->getType().isAddress());
     return insert(new (getModule())
                       BeginBorrowInst(getSILDebugLocation(Loc), LV, isLexical,
-                                      hasPointerEscape, fromVarDecl));
+                                      hasPointerEscape, fromVarDecl, fixed));
   }
 
   /// Convenience function for creating a load_borrow on non-trivial values and
diff --git a/include/swift/SIL/SILInstruction.h b/include/swift/SIL/SILInstruction.h
@@ -4524,15 +4524,18 @@ class BeginBorrowInst
   USE_SHARED_UINT8;
 
   BeginBorrowInst(SILDebugLocation DebugLoc, SILValue LValue, bool isLexical,
-                  bool hasPointerEscape, bool fromVarDecl)
+                  bool hasPointerEscape, bool fromVarDecl, bool fixed)
       : UnaryInstructionBase(DebugLoc, LValue,
                              LValue->getType().getObjectType()) {
     sharedUInt8().BeginBorrowInst.lexical = isLexical;
     sharedUInt8().BeginBorrowInst.pointerEscape = hasPointerEscape;
     sharedUInt8().BeginBorrowInst.fromVarDecl = fromVarDecl;
+    sharedUInt8().BeginBorrowInst.fixed = fixed;
   }
 
 public:
+  
+
   // FIXME: this does not return all instructions that end a local borrow
   // scope. Branches can also end it via a reborrow, so APIs using this are
   // incorrect. Instead, either iterate over all uses and return those with
@@ -4560,6 +4563,12 @@ class BeginBorrowInst
     return sharedUInt8().BeginBorrowInst.fromVarDecl;
   }
 
+  /// Whether the borrow scope is fixed during move checking and should be
+  /// treated as an opaque use of the value.
+  bool isFixed() const {
+    return sharedUInt8().BeginBorrowInst.fixed;
+  }
+
   /// Return a range over all EndBorrow instructions for this BeginBorrow.
   EndBorrowRange getEndBorrows() const;
 
diff --git a/include/swift/SIL/SILNode.h b/include/swift/SIL/SILNode.h
@@ -243,7 +243,8 @@ class alignas(8) SILNode :
     SHARED_FIELD(BeginBorrowInst, uint8_t
                  lexical : 1,
                  pointerEscape : 1,
-                 fromVarDecl : 1);
+                 fromVarDecl : 1,
+                 fixed : 1);
 
     SHARED_FIELD(CopyAddrInst, uint8_t
       isTakeOfSrc : 1,
diff --git a/lib/SIL/IR/SILPrinter.cpp b/lib/SIL/IR/SILPrinter.cpp
@@ -1737,6 +1737,9 @@ class SILPrinter : public SILInstructionVisitor<SILPrinter> {
     if (BBI->isFromVarDecl()) {
       *this << "[var_decl] ";
     }
+    if (BBI->isFixed()) {
+      *this << "[fixed] ";
+    }
     *this << getIDAndType(BBI->getOperand());
   }
 
diff --git a/lib/SIL/Parser/ParseSIL.cpp b/lib/SIL/Parser/ParseSIL.cpp
@@ -3711,6 +3711,7 @@ bool SILParser::parseSpecificSILInstruction(SILBuilder &B,
     bool isLexical = false;
     bool hasPointerEscape = false;
     bool fromVarDecl = false;
+    bool fixed = false;
 
     StringRef AttrName;
     SourceLoc AttrLoc;
@@ -3721,6 +3722,8 @@ bool SILParser::parseSpecificSILInstruction(SILBuilder &B,
         hasPointerEscape = true;
       else if (AttrName == "var_decl")
         fromVarDecl = true;
+      else if (AttrName == "fixed")
+        fixed = true;
       else {
         P.diagnose(InstLoc.getSourceLoc(),
                    diag::sil_invalid_attribute_for_instruction, AttrName,
@@ -3734,7 +3737,7 @@ bool SILParser::parseSpecificSILInstruction(SILBuilder &B,
       return true;
 
     ResultVal = B.createBeginBorrow(InstLoc, Val, isLexical, hasPointerEscape,
-                                    fromVarDecl);
+                                    fromVarDecl, fixed);
     break;
   }
 
diff --git a/lib/SILGen/SILGenBuilder.cpp b/lib/SILGen/SILGenBuilder.cpp
@@ -1038,13 +1038,26 @@ ManagedValue SILGenBuilder::createMarkDependence(
 
 ManagedValue SILGenBuilder::createBeginBorrow(SILLocation loc,
                                               ManagedValue value,
-                                              bool isLexical) {
+                                              bool isLexical,
+                                              bool isFixed) {
   auto *newValue =
-      SILBuilder::createBeginBorrow(loc, value.getValue(), isLexical);
+      SILBuilder::createBeginBorrow(loc, value.getValue(),
+                                    isLexical, false, false, isFixed);
   SGF.emitManagedBorrowedRValueWithCleanup(newValue);
   return ManagedValue::forBorrowedObjectRValue(newValue);
 }
 
+ManagedValue SILGenBuilder::createFormalAccessBeginBorrow(SILLocation loc,
+                                              ManagedValue value,
+                                              bool isLexical,
+                                              bool isFixed) {
+  auto *newValue =
+      SILBuilder::createBeginBorrow(loc, value.getValue(),
+                                    isLexical, false, false, isFixed);
+  return SGF.emitFormalEvaluationManagedBorrowedRValueWithCleanup(loc,
+                                                    value.getValue(), newValue);
+}
+
 ManagedValue SILGenBuilder::createMoveValue(SILLocation loc, ManagedValue value,
                                             bool isLexical) {
   assert(value.isPlusOne(SGF) && "Must be +1 to be moved!");
diff --git a/lib/SILGen/SILGenBuilder.h b/lib/SILGen/SILGenBuilder.h
@@ -470,7 +470,13 @@ class SILGenBuilder : public SILBuilder {
 
   using SILBuilder::createBeginBorrow;
   ManagedValue createBeginBorrow(SILLocation loc, ManagedValue value,
-                                 bool isLexical = false);
+                                 bool isLexical = false,
+                                 bool isFixed = false);
+
+  ManagedValue createFormalAccessBeginBorrow(SILLocation loc,
+                                             ManagedValue value,
+                                             bool isLexical = false,
+                                             bool isFixed = false);
 
   using SILBuilder::createMoveValue;
   ManagedValue createMoveValue(SILLocation loc, ManagedValue value,
diff --git a/lib/SILGen/SILGenPattern.cpp b/lib/SILGen/SILGenPattern.cpp
@@ -3425,26 +3425,29 @@ void SILGenFunction::emitSwitchStmt(SwitchStmt *S) {
         
         case ValueOwnership::Shared:
           emission.setNoncopyableBorrowingOwnership();
-          if (!subjectMV.isPlusZero()) {
-            subjectMV = subjectMV.borrow(*this, S);
-          }
           if (subjectMV.getType().isAddress()) {
+            // Initiate a read access on the memory, to ensure that even
+            // if the underlying memory is mutable or consumable, the pattern
+            // match is not allowed to modify it.
+            auto access = B.createBeginAccess(S, subjectMV.getValue(),
+              SILAccessKind::Read,
+              SILAccessEnforcement::Static,
+              /*no nested conflict*/ true, false);
+            Cleanups.pushCleanup<EndAccessCleanup>(access);
+            subjectMV = ManagedValue::forBorrowedAddressRValue(access);
             if (subjectMV.getType().isLoadable(F)) {
               // Load a borrow if the type is loadable.
               subjectMV = subjectUndergoesFormalAccess
                 ? B.createFormalAccessLoadBorrow(S, subjectMV)
                 : B.createLoadBorrow(S, subjectMV);
-            } else {
-              // Initiate a read access on the memory, to ensure that even
-              // if the underlying memory is mutable or consumable, the pattern
-              // match is not allowed to modify it.
-              auto access = B.createBeginAccess(S, subjectMV.getValue(),
-                SILAccessKind::Read,
-                SILAccessEnforcement::Static,
-                /*no nested conflict*/ true, false);
-              Cleanups.pushCleanup<EndAccessCleanup>(access);
-              subjectMV = ManagedValue::forBorrowedAddressRValue(access);
             }
+          } else {
+            // Initiate a fixed borrow on the subject, so that it's treated as
+            // opaque by the move checker.
+            subjectMV = subjectUndergoesFormalAccess
+              ? B.createFormalAccessBeginBorrow(S, subjectMV,
+                                                false, /*fixed*/true)
+              : B.createBeginBorrow(S, subjectMV, false, /*fixed*/ true);
           }
           return {subjectMV, CastConsumptionKind::BorrowAlways};
           
@@ -3467,8 +3470,19 @@ void SILGenFunction::emitSwitchStmt(SwitchStmt *S) {
             Cleanups.getCleanupsDepth(),
             subjectMV);
 
-          // Perform the pattern match on a borrow of the subject.
-          subjectMV = subjectMV.borrow(*this, S);
+          // Perform the pattern match on an opaque borrow or read access of the
+          // subject.
+          if (subjectMV.getType().isAddress()) {
+            auto access = B.createBeginAccess(S, subjectMV.getValue(),
+              SILAccessKind::Read,
+              SILAccessEnforcement::Static,
+              /*no nested conflict*/ true, false);
+            Cleanups.pushCleanup<EndAccessCleanup>(access);
+            subjectMV = ManagedValue::forBorrowedAddressRValue(access);
+          } else {
+            subjectMV = B.createBeginBorrow(S, subjectMV,
+                                            false, /*fixed*/ true);
+          }
           return {subjectMV, CastConsumptionKind::BorrowAlways};
         }
         llvm_unreachable("unhandled value ownership");
diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureUtils.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureUtils.cpp
@@ -435,8 +435,9 @@ bool Implementation::gatherUses(SILValue value) {
       continue;
     }
     case OperandOwnership::Borrow: {
-      // Look through borrows.
-      if (auto *bbi = dyn_cast<BeginBorrowInst>(nextUse->getUser())) {
+      if (auto *bbi = dyn_cast<BeginBorrowInst>(nextUse->getUser());
+          bbi && !bbi->isFixed()) {
+        // Look through non-fixed borrows.
         LLVM_DEBUG(llvm::dbgs() << "        Found recursive borrow!\n");
         for (auto *use : bbi->getUses()) {
           useWorklist.push_back(use);
@@ -452,7 +453,7 @@ bool Implementation::gatherUses(SILValue value) {
       }
 
       // Otherwise, treat it as a normal use.
-      LLVM_DEBUG(llvm::dbgs() << "        Treating non-begin_borrow borrow as "
+      LLVM_DEBUG(llvm::dbgs() << "        Treating borrow as "
                                  "a non lifetime ending use!\n");
       blocksToUses.insert(nextUse->getParentBlock(),
                           {nextUse,
diff --git a/lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp b/lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp
@@ -484,11 +484,18 @@ eliminateSimpleBorrows(BeginBorrowInst *bbi, CanonicalizeInstruction &pass) {
   if (bbi->isLexical() && (bbi->getModule().getStage() == SILStage::Raw ||
                            !isNestedLexicalBeginBorrow(bbi)))
     return next;
+    
+  // Fixed borrow scopes can't be eliminated during the raw stage since they
+  // control move checker behavior.
+  if (bbi->isFixed() && bbi->getModule().getStage() == SILStage::Raw) {
+    return next;
+  }
 
   // Borrow scopes representing a VarDecl can't be eliminated during the raw
   // stage because they may be needed for diagnostics.
-  if (bbi->isFromVarDecl() && (bbi->getModule().getStage() == SILStage::Raw))
+  if (bbi->isFromVarDecl() && bbi->getModule().getStage() == SILStage::Raw) {
     return next;
+  }
 
   // We know that our borrow is completely within the lifetime of its base value
   // if the borrow is never reborrowed. We check for reborrows and do not
diff --git a/lib/Serialization/DeserializeSIL.cpp b/lib/Serialization/DeserializeSIL.cpp
@@ -2224,11 +2224,12 @@ bool SILDeserializer::readSILInstruction(SILFunction *Fn,
     bool isLexical = Attr & 0x1;
     bool hasPointerEscape = (Attr >> 1) & 0x1;
     bool fromVarDecl = (Attr >> 2) & 0x1;
+    bool isFixed = (Attr >> 3) & 0x1;
     ResultInst = Builder.createBeginBorrow(
         Loc,
         getLocalValue(ValID, getSILType(MF->getType(TyID),
                                         (SILValueCategory)TyCategory, Fn)),
-        isLexical, hasPointerEscape, fromVarDecl);
+        isLexical, hasPointerEscape, fromVarDecl, isFixed);
     break;
   }
 
diff --git a/lib/Serialization/SerializeSIL.cpp b/lib/Serialization/SerializeSIL.cpp
@@ -1572,7 +1572,8 @@ void SILSerializer::writeSILInstruction(const SILInstruction &SI) {
     } else if (auto *BBI = dyn_cast<BeginBorrowInst>(&SI)) {
       Attr = unsigned(BBI->isLexical()) |
              (unsigned(BBI->hasPointerEscape() << 1)) |
-             (unsigned(BBI->isFromVarDecl() << 2));
+             (unsigned(BBI->isFromVarDecl() << 2)) |
+             (unsigned(BBI->isFixed() << 3));
     } else if (auto *MVI = dyn_cast<MoveValueInst>(&SI)) {
       Attr = unsigned(MVI->getAllowDiagnostics()) |
              (unsigned(MVI->isLexical() << 1)) |
diff --git a/test/SILGen/borrowing_switch_subjects.swift b/test/SILGen/borrowing_switch_subjects.swift
@@ -34,7 +34,8 @@ func borrowParam(x: borrowing Outer) {
     
     // CHECK: [[BORROW_OUTER:%.*]] = begin_borrow {{.*}} : $Outer
     // CHECK: [[BORROW_INNER:%.*]] = struct_extract [[BORROW_OUTER]]
-    // CHECK: [[COPY_INNER:%.*]] = copy_value [[BORROW_INNER]]
+    // CHECK: [[BORROW_FIX:%.*]] = begin_borrow [fixed] [[BORROW_INNER]]
+    // CHECK: [[COPY_INNER:%.*]] = copy_value [[BORROW_FIX]]
     // CHECK: [[MARK:%.*]] = mark_unresolved_non_copyable_value [strict] [no_consume_or_assign] [[COPY_INNER]]
     // CHECK: [[BORROW:%.*]] = begin_borrow [[MARK]]
     switch x.storedInner {
@@ -50,7 +51,8 @@ func borrowParam(x: borrowing Outer) {
     // CHECK: [[COPY_INNER:%.*]] = copy_value [[BORROW_INNER]]
     // CHECK: [[MARK:%.*]] = mark_unresolved_non_copyable_value [no_consume_or_assign] [[COPY_INNER]]
     // CHECK: [[BORROW:%.*]] = begin_borrow [[MARK]]
-    // CHECK: [[COPY2:%.*]] = copy_value [[BORROW]]
+    // CHECK: [[BORROW_FIX:%.*]] = begin_borrow [fixed] [[BORROW]]
+    // CHECK: [[COPY2:%.*]] = copy_value [[BORROW_FIX]]
     // CHECK: [[MARK2:%.*]] = mark_unresolved_non_copyable_value [strict] [no_consume_or_assign] [[COPY2]]
     // CHECK: [[BORROW2:%.*]] = begin_borrow [[MARK2]]
     switch x.readInner {
@@ -63,7 +65,7 @@ func borrowParam(x: borrowing Outer) {
 
     // CHECK: [[FN:%.*]] = function_ref @{{.*}}9temporary
     // CHECK: [[TMP:%.*]] = apply [[FN]]()
-    // CHECK: [[BORROW_OUTER:%.*]] = begin_borrow [[TMP]]
+    // CHECK: [[BORROW_OUTER:%.*]] = begin_borrow [fixed] [[TMP]]
     // CHECK: [[COPY:%.*]] = copy_value [[BORROW_OUTER]]
     // CHECK: [[MARK:%.*]] = mark_unresolved_non_copyable_value [strict] [no_consume_or_assign] [[COPY]]
     // CHECK: [[BORROW:%.*]] = begin_borrow [[MARK]]
diff --git a/test/SILOptimizer/moveonly_borrowing_switch_yield.swift b/test/SILOptimizer/moveonly_borrowing_switch_yield.swift
@@ -3,9 +3,12 @@
 extension List {
     var peek: Element {
         _read {
-            // TODO: fix move-only checker induced ownership bug when
-            // we try to switch over `self.head` here.
-            yield head.peek
+            switch self.head {
+            case .empty: fatalError()
+            case .more(_borrowing box):
+                yield box.wrapped.element
+            }
+            //yield head.peek
         }
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -1737,6 +1737,9 @@ class SILPrinter : public SILInstructionVisitor<SILPrinter> {`
`1737`	`1737`	`if (BBI->isFromVarDecl()) {`
`1738`	`1738`	`*this << "[var_decl] ";`
`1739`	`1739`	`}`
	`1740`	`+ if (BBI->isFixed()) {`
	`1741`	`+ *this << "[fixed] ";`
	`1742`	`+ }`
`1740`	`1743`	`*this << getIDAndType(BBI->getOperand());`
`1741`	`1744`	`}`
`1742`	`1745`