Merge pull request #70333 from jckarter/read-coroutines-yielding-noncopyable-values

Move-only-check the yielded result from read coroutines when they're noncopyable.

Author: jckarter

lib/SILGen/SILGenApply.cpp

@@ -4822,17 +4822,28 @@ namespace {
 /// Cleanup to end a coroutine application.
 class EndCoroutineApply : public Cleanup {
   SILValue ApplyToken;
+  std::vector<BeginBorrowInst *> BorrowedMoveOnlyValues;
 public:
   EndCoroutineApply(SILValue applyToken) : ApplyToken(applyToken) {}
 
+  void setBorrowedMoveOnlyValues(ArrayRef<BeginBorrowInst *> values) {
+    BorrowedMoveOnlyValues.insert(BorrowedMoveOnlyValues.end(),
+                                  values.begin(), values.end());
+  }
+
   void emit(SILGenFunction &SGF, CleanupLocation l, ForUnwind_t forUnwind) override {
+    for (auto i = BorrowedMoveOnlyValues.rbegin(), e = BorrowedMoveOnlyValues.rend();
+         i != e; ++i) {
+      SGF.B.createEndBorrow(l, *i);
+      SGF.B.createDestroyValue(l, (*i)->getOperand());
+    }
     if (forUnwind) {
       SGF.B.createAbortApply(l, ApplyToken);
     } else {
       SGF.B.createEndApply(l, ApplyToken);
     }
   }
-
+  
   void dump(SILGenFunction &SGF) const override {
 #ifndef NDEBUG
     llvm::errs() << "EndCoroutineApply "
@@ -4897,23 +4908,51 @@ SILGenFunction::emitBeginApply(SILLocation loc, ManagedValue fn,
   auto yieldInfos = substFnType->getYields();
   assert(yieldValues.size() == yieldInfos.size());
   bool useLoweredAddresses = silConv.useLoweredAddresses();
+  SmallVector<BeginBorrowInst *, 2> borrowedMoveOnlyValues;
   for (auto i : indices(yieldValues)) {
     auto value = yieldValues[i];
     auto info = yieldInfos[i];
     if (info.isIndirectInOut()) {
       yields.push_back(ManagedValue::forLValue(value));
     } else if (info.isConsumed()) {
+      if (value->getType().isMoveOnly()) {
+        value = B.createMarkUnresolvedNonCopyableValueInst(loc, value,
+         MarkUnresolvedNonCopyableValueInst::CheckKind::ConsumableAndAssignable);
+      }
       !useLoweredAddresses && value->getType().isTrivial(getFunction())
           ? yields.push_back(ManagedValue::forRValueWithoutOwnership(value))
           : yields.push_back(emitManagedRValueWithCleanup(value));
     } else if (info.isGuaranteed()) {
-      !useLoweredAddresses && value->getType().isTrivial(getFunction())
-          ? yields.push_back(ManagedValue::forRValueWithoutOwnership(value))
-          : yields.push_back(ManagedValue::forBorrowedRValue(value));
+      if (value->getType().isMoveOnly()) {
+        if (!value->getType().isAddress()) {
+          // The move checker uses the lifetime of the "copy" for borrow checking.
+          value = B.createCopyValue(loc, value);
+          value = B.createMarkUnresolvedNonCopyableValueInst(loc, value,
+            MarkUnresolvedNonCopyableValueInst::CheckKind::NoConsumeOrAssign);
+          auto borrow = B.createBeginBorrow(loc, value);
+          yields.push_back(ManagedValue::forBorrowedRValue(borrow));
+          borrowedMoveOnlyValues.push_back(borrow);
+        } else {
+          value = B.createMarkUnresolvedNonCopyableValueInst(loc, value,
+            MarkUnresolvedNonCopyableValueInst::CheckKind::NoConsumeOrAssign);
+          yields.push_back(ManagedValue::forRValueWithoutOwnership(value));
+        }
+      } else {
+        !useLoweredAddresses && value->getType().isTrivial(getFunction())
+            ? yields.push_back(ManagedValue::forRValueWithoutOwnership(value))
+            : yields.push_back(ManagedValue::forBorrowedRValue(value));
+      }
     } else {
+      assert(!value->getType().isMoveOnly()
+             && "move-only types shouldn't be trivial");
       yields.push_back(ManagedValue::forRValueWithoutOwnership(value));
     }
   }
+  
+  if (!borrowedMoveOnlyValues.empty()) {
+    auto &endApply = static_cast<EndCoroutineApply &>(Cleanups.getCleanup(endApplyHandle));
+    endApply.setBorrowedMoveOnlyValues(borrowedMoveOnlyValues);
+  }
 
   return endApplyHandle;
 }

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

@@ -950,6 +950,9 @@ void UseState::initializeLiveness(
                              reinitInstAndValue.second);
     }
   }
+  
+  // FIXME: Whether the initial use is an initialization ought to be entirely
+  // derivable from the CheckKind of the mark instruction.
 
   // Then check if our markedValue is from an argument that is in,
   // in_guaranteed, inout, or inout_aliasable, consider the marked address to be
@@ -1048,6 +1051,12 @@ void UseState::initializeLiveness(
     recordInitUse(address, address, liveness.getTopLevelSpan());
     liveness.initializeDef(address, liveness.getTopLevelSpan());
   }
+  
+  if (auto *bai = dyn_cast_or_null<BeginApplyInst>(
+        stripAccessMarkers(address->getOperand())->getDefiningInstruction())) {
+    recordInitUse(address, address, liveness.getTopLevelSpan());
+    liveness.initializeDef(address, liveness.getTopLevelSpan());
+  }
 
   // Now that we have finished initialization of defs, change our multi-maps
   // from their array form to their map form.

lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp

@@ -113,6 +113,21 @@ static void getVariableNameForValue(SILValue value2,
       break;
     }
 
+    if (auto bai = dyn_cast_or_null<BeginApplyInst>(searchValue->getDefiningInstruction())) {
+      // Use the name of the property being accessed if we can get to it.
+      if (isa<FunctionRefBaseInst>(bai->getCallee())
+          || isa<MethodInst>(bai->getCallee())) {
+        variableNamePath.push_back(bai->getCallee()->getDefiningInstruction());
+        // Try to name the base of the property if this is a method.
+        if (bai->getSubstCalleeType()->hasSelfParam()) {
+          searchValue = bai->getSelfArgument();
+          continue;
+        } else {
+          break;
+        }
+      }
+    }
+
     // If we do not do an exact match, see if we can find a debug_var inst. If
     // we do, we always break since we have a root value.
     if (auto *use = getAnyDebugUse(searchValue)) {
@@ -132,12 +147,25 @@ static void getVariableNameForValue(SILValue value2,
       searchValue = cast<SingleValueInstruction>(searchValue)->getOperand(0);
       continue;
     }
-
+    
     // If we do not pattern match successfully, just set resulting string to
     // unknown and return early.
     resultingString += "unknown";
     return;
   }
+  
+  auto nameFromDecl = [&](Decl *d) -> StringRef {
+    if (d) {
+      if (auto accessor = dyn_cast<AccessorDecl>(d)) {
+        return accessor->getStorage()->getBaseName().userFacingName();
+      }
+      if (auto vd = dyn_cast<ValueDecl>(d)) {
+        return vd->getBaseName().userFacingName();
+      }
+    }
+    
+    return "<unknown decl>";
+  };
 
   // Walk backwards, constructing our string.
   while (true) {
@@ -148,6 +176,16 @@ static void getVariableNameForValue(SILValue value2,
         resultingString += i.getName();
       } else if (auto i = VarDeclCarryingInst(inst)) {
         resultingString += i.getName();
+      } else if (auto f = dyn_cast<FunctionRefBaseInst>(inst)) {
+        if (auto dc = f->getInitiallyReferencedFunction()->getDeclContext()) {
+          resultingString += nameFromDecl(dc->getAsDecl());
+        } else {
+          resultingString += "<unknown decl>";
+        }
+      } else if (auto m = dyn_cast<MethodInst>(inst)) {
+        resultingString += nameFromDecl(m->getMember().getDecl());
+      } else {
+        resultingString += "<unknown decl>";      
       }
     } else {
       auto value = next.get<SILValue>();

lib/SILOptimizer/Mandatory/MoveOnlyObjectCheckerUtils.cpp

@@ -481,6 +481,11 @@ void MoveOnlyObjectCheckerPImpl::check(DominanceInfo *domTree,
             i = copyToMoveOnly;
           }
 
+          // TODO: Instead of pattern matching specific code generation patterns,
+          // we should be able to eliminate any `copy_value` whose canonical
+          // lifetime fits within the borrow scope of the borrowed value being
+          // copied from.
+
           // Handle:
           //
           // bb0(%0 : @guaranteed $Type):
@@ -521,6 +526,24 @@ void MoveOnlyObjectCheckerPImpl::check(DominanceInfo *domTree,
               }
             }
           }
+          
+          // Handle:
+          //   (%yield, ..., %handle) = begin_apply
+          //   %copy = copy_value %yield
+          //   %mark = mark_unresolved_noncopyable_value [no_consume_or_assign] %copy
+          if (auto bai = dyn_cast_or_null<BeginApplyInst>(i->getOperand(0)->getDefiningInstruction())) {
+            if (i->getOperand(0)->getOwnershipKind() == OwnershipKind::Guaranteed) {
+              for (auto *use : markedInst->getConsumingUses()) {
+                destroys.push_back(cast<DestroyValueInst>(use->getUser()));
+              }
+              while (!destroys.empty())
+                destroys.pop_back_val()->eraseFromParent();
+              markedInst->replaceAllUsesWith(i->getOperand(0));
+              markedInst->eraseFromParent();
+              cvi->eraseFromParent();
+              continue;
+            }
+          }
         }
       }
     }