Merge pull request #84830 from gottesmm/pr-280fbe6eca8e73a6caec46dd571aa9afe8bf8b93

[rbi] Cleanup handling of how we send parameters to fix issues with inout sending.

Author: gottesmm

include/swift/SIL/ApplySite.h

@@ -645,14 +645,27 @@ class ApplySite {
     return getSubstCalleeConv().getParamInfoForSILArg(calleeArgIndex);
   }
 
+  /// Returns true if \p op is the callee operand of this apply site
+  /// and not an argument operand.
+  ///
+  /// If this instruction is not a full apply site, this always returns false.
+  bool isCalleeOperand(const Operand &op) const;
+
+  /// Returns true if this is an 'out' parameter.
   bool isSending(const Operand &oper) const {
-    if (isIndirectErrorResultOperand(oper))
+    if (isIndirectErrorResultOperand(oper) || oper.isTypeDependent() ||
+        isCalleeOperand(oper))
       return false;
     if (isIndirectResultOperand(oper))
       return getSubstCalleeType()->hasSendingResult();
     return getArgumentParameterInfo(oper).hasOption(SILParameterInfo::Sending);
   }
 
+  /// Returns true if this operand is an 'inout sending' parameter.
+  bool isInOutSending(const Operand &oper) const {
+    return isSending(oper) && getArgumentConvention(oper).isInoutConvention();
+  }
+
   /// Return true if 'operand' is addressable after type substitution in the
   /// caller's context.
   bool isAddressable(const Operand &operand) const;
@@ -1056,6 +1069,13 @@ inline bool ApplySite::isIndirectErrorResultOperand(const Operand &op) const {
   return fas.isIndirectErrorResultOperand(op);
 }
 
+inline bool ApplySite::isCalleeOperand(const Operand &op) const {
+  auto fas = asFullApplySite();
+  if (!fas)
+    return false;
+  return fas.isCalleeOperand(op);
+}
+
 } // namespace swift
 
 #endif

lib/SILOptimizer/Analysis/RegionAnalysis.cpp

@@ -1723,9 +1723,13 @@ struct PartitionOpBuilder {
         PartitionOp::Send(lookupValueID(representative), op));
   }
 
-  void addUndoSend(SILValue representative, SILInstruction *unsendingInst) {
+  void addUndoSend(TrackableValue value, SILInstruction *unsendingInst) {
+    if (value.isSendable())
+      return;
+
+    auto representative = value.getRepresentative().getValue();
     assert(valueHasID(representative) &&
-           "value should already have been encountered");
+           "sent value should already have been encountered");
 
     currentInstPartitionOps->emplace_back(
         PartitionOp::UndoSend(lookupValueID(representative), unsendingInst));
@@ -2430,7 +2434,7 @@ class PartitionOpTranslator {
                 }))
           continue;
 
-        builder.addUndoSend(trackedArgValue.getRepresentative().getValue(), ai);
+        builder.addUndoSend(trackedArgValue, ai);
       }
     }
   }
@@ -2567,60 +2571,53 @@ class PartitionOpTranslator {
     // gather our non-sending parameters.
     SmallVector<Operand *, 8> nonSendingParameters;
     SmallVector<Operand *, 8> sendingIndirectResults;
-    if (fas.getNumArguments()) {
-      // NOTE: We want to process indirect parameters as if they are
-      // parameters... so we process them in nonSendingParameters.
-      for (auto &op : fas.getOperandsWithoutSelf()) {
-        // If op is the callee operand, skip it.
-        if (fas.isCalleeOperand(op))
-          continue;
 
-        if (fas.isSending(op)) {
-          if (fas.isIndirectResultOperand(op)) {
-            sendingIndirectResults.push_back(&op);
-            continue;
-          }
+    // NOTE: We want to process indirect parameters as if they are
+    // parameters... so we process them in nonSendingParameters.
+    for (auto &op : fas->getAllOperands()) {
+      // If op is the callee operand or type dependent operand, skip it.
+      if (op.isTypeDependent())
+        continue;
 
-          // Attempt to lookup the value we are passing as sending. We want to
-          // require/send value if it is non-Sendable and require its base if it
-          // is non-Sendable as well.
-          if (auto lookupResult = tryToTrackValue(op.get())) {
-            builder.addRequire(*lookupResult);
-            builder.addSend(lookupResult->value, &op);
-          }
-        } else {
-          nonSendingParameters.push_back(&op);
+      if (fas.isCalleeOperand(op)) {
+        if (auto calleeResult = tryToTrackValue(op.get())) {
+          builder.addRequire(*calleeResult);
         }
+        continue;
       }
-    }
 
-    // If our self parameter was sending, send it. Otherwise, just
-    // stick it in the non self operand values array and run multiassign on
-    // it.
-    if (fas.hasSelfArgument()) {
-      auto &selfOperand = fas.getSelfArgumentOperand();
-      if (fas.getArgumentParameterInfo(selfOperand)
-              .hasOption(SILParameterInfo::Sending)) {
-        if (auto lookupResult = tryToTrackValue(selfOperand.get())) {
-          builder.addRequire(*lookupResult);
-          builder.addSend(lookupResult->value, &selfOperand);
-        }
-      } else {
-        nonSendingParameters.push_back(&selfOperand);
+      // If our parameter is not sending, just add it to the non-sending
+      // parameters array and continue.
+      if (!fas.isSending(op)) {
+        nonSendingParameters.push_back(&op);
+        continue;
       }
-    }
 
-    // Require our callee operand if it is non-Sendable.
-    //
-    // DISCUSSION: Even though we do not include our callee operand in the same
-    // region as our operands/results, we still need to require that it is live
-    // at the point of application. Otherwise, we will not emit errors if the
-    // closure before this function application is already in the same region as
-    // a sent value. In such a case, the function application must error.
-    if (auto calleeResult = tryToTrackValue(fas.getCallee())) {
-      builder.addRequire(*calleeResult);
+      // Otherwise, first handle indirect result operands.
+      if (fas.isIndirectResultOperand(op)) {
+        sendingIndirectResults.push_back(&op);
+        continue;
+      }
+
+      // Attempt to lookup the value we are passing as sending. We want to
+      // require/send value if it is non-Sendable and require its base if it
+      // is non-Sendable as well.
+      if (auto lookupResult = tryToTrackValue(op.get())) {
+        builder.addRequire(*lookupResult);
+        builder.addSend(lookupResult->value, &op);
+      }
     }
 
+    SWIFT_DEFER {
+      for (auto &op : fas->getAllOperands()) {
+        if (!fas.isInOutSending(op))
+          continue;
+        if (auto lookupResult = tryToTrackValue(op.get())) {
+          builder.addUndoSend(lookupResult->value, op.getUser());
+        }
+      }
+    };
+
     SmallVector<SILValue, 8> applyResults;
     getApplyResults(*fas, applyResults);
 
@@ -2688,36 +2685,77 @@ class PartitionOpTranslator {
 
   /// Handles the semantics for SIL applies that cross isolation.
   ///
-  /// Semantically this causes all arguments of the applysite to be sent.
+  /// Semantically we are attempting to implement the following:
+  ///
+  /// * Step 1: Require all non-sending parameters and then send those
+  ///   parameters. We perform all of the requires first and the sends second
+  ///   since all of the parameters are getting sent to the same isolation
+  ///   domains and become part of the same region in our callee. So in a
+  ///   certain sense, we are performing a require over the entire merge of the
+  ///   parameter regions and then send each constituant part of the region
+  ///   without requiring again so we do not emit use-after-send diagnostics.
+  ///
+  /// * Step 2: Require/Send each of the sending parameters one by one. This
+  ///   includes both 'sending' and 'inout sending' parameters. We purposely
+  ///   interleave the require/send operations to ensure that if one passes a
+  ///   value twice to different 'sending' or 'inout sending' parameters, we
+  ///   will emit an error.
+  ///
+  /// * Step 3: Unsend each of the unsending parameters. Since our caller
+  ///   ensures that 'inout sending' parameters are disconnected on return and
+  ///   are in different regions from all other parameters, we can just simply
+  ///   unsend the parameter so we can use it again later.
   void translateIsolationCrossingSILApply(FullApplySite applySite) {
-    // Require all operands first before we emit a send.
-    for (auto op : applySite.getArguments()) {
-      if (auto lookupResult = tryToTrackValue(op)) {
+    SmallVector<TrackableValue, 8> inoutSendingParams;
+
+    // First go through and require all of our operands that are not 'sending'
+    for (auto &op : applySite.getArgumentOperands()) {
+      if (applySite.isSending(op))
+        continue;
+      if (auto lookupResult = tryToTrackValue(op.get()))
         builder.addRequire(*lookupResult);
-      }
     }
 
-    auto handleSILOperands = [&](MutableArrayRef<Operand> operands) {
-      for (auto &op : operands) {
-        if (auto lookupResult = tryToTrackValue(op.get())) {
-          builder.addSend(lookupResult->value, &op);
-        }
+    // Then go through our operands again and send all of our non-sending
+    // parameters. We do not interleave these sends with our requires since we
+    // are considering these values to be merged into the same region. We could
+    // also merge them in the caller but there is no point in doing so
+    // semantically since the values cannot be used again locally.
+    for (auto &op : applySite.getOperandsWithoutIndirectResults()) {
+      if (applySite.isSending(op))
+        continue;
+      if (auto lookupResult = tryToTrackValue(op.get())) {
+        builder.addSend(lookupResult->value, &op);
       }
     };
 
-    auto handleSILSelf = [&](Operand *self) {
-      if (auto lookupResult = tryToTrackValue(self->get())) {
-        builder.addSend(lookupResult->value, self);
+    // Then go through our 'sending' params and require/send each in sequence.
+    //
+    // We do this interleaved so that if a value is passed to multiple 'sending'
+    // parameters, we emit errors.
+    for (auto &op : applySite.getOperandsWithoutIndirectResults()) {
+      if (!applySite.isSending(op))
+        continue;
+      auto lookupResult = tryToTrackValue(op.get());
+      if (!lookupResult)
+        continue;
+      builder.addRequire(*lookupResult);
+      builder.addSend(lookupResult->value, &op);
+    }
+
+    // Now use a SWIFT_DEFER so that when the function is done executing, we
+    // unsend 'inout sending' params.
+    SWIFT_DEFER {
+      for (auto &op : applySite.getOperandsWithoutIndirectResults()) {
+        if (!applySite.isInOutSending(op))
+          continue;
+        auto lookupResult = tryToTrackValue(op.get());
+        if (!lookupResult)
+          continue;
+        builder.addUndoSend(lookupResult->value, *applySite);
       }
     };
 
-    if (applySite.hasSelfArgument()) {
-      handleSILOperands(applySite.getOperandsWithoutIndirectResultsOrSelf());
-      handleSILSelf(&applySite.getSelfArgumentOperand());
-    } else {
-      handleSILOperands(applySite.getOperandsWithoutIndirectResults());
-    }
-
     // Create a new assign fresh for each one of our values and unless our
     // return value is sending, emit an extra error bit on the results that are
     // non-Sendable.