[region-isolation] More consuming -> transferring.

gottesmm · gottesmm · commit 382609e3ac81 · 2023-10-26T12:29:23.000-07:00
diff --git a/include/swift/SILOptimizer/Utils/PartitionUtils.h b/include/swift/SILOptimizer/Utils/PartitionUtils.h
@@ -83,7 +83,7 @@ enum class PartitionOpKind : uint8_t {
   /// non-transferred regions.
   Merge,
 
-  /// Consume the region of a value if not already transferred, takes one arg.
+  /// Transfer the region of a value if not already transferred, takes one arg.
   Transfer,
 
   /// Require the region of a value to be non-transferred, takes one arg.
@@ -104,7 +104,7 @@ class PartitionOp {
   SILInstruction *sourceInst;
 
   /// Record an AST expression corresponding to this PartitionOp, currently
-  /// populated only for Consume expressions to indicate the value being
+  /// populated only for Transfer expressions to indicate the value being
   /// transferred
   Expr *sourceExpr;
 
@@ -381,17 +381,17 @@ class Partition {
   /// `handleFailure` closure can optionally be passed in that will be called if
   /// a transferred region is required. The closure is given the PartitionOp
   /// that failed, and the index of the SIL value that was required but
-  /// transferred. Additionally, a list of "nonconsumable" indices can be passed
-  /// in along with a handleConsumeNonConsumable closure. In the event that a
-  /// region containing one of the nonconsumable indices is transferred, the
-  /// closure will be called with the offending transfer.
+  /// transferred. Additionally, a list of "nontransferrable" indices can be
+  /// passed in along with a handleTransferNonTransferrable closure. In the
+  /// event that a region containing one of the nontransferrable indices is
+  /// transferred, the closure will be called with the offending transfer.
   void apply(
       PartitionOp op,
       llvm::function_ref<void(const PartitionOp &, Element)> handleFailure =
           [](const PartitionOp &, Element) {},
-      ArrayRef<Element> nonconsumables = {},
+      ArrayRef<Element> nontransferrables = {},
       llvm::function_ref<void(const PartitionOp &, Element)>
-          handleConsumeNonConsumable = [](const PartitionOp &, Element) {},
+          handleTransferNonTransferrable = [](const PartitionOp &, Element) {},
       llvm::function_ref<bool(Element)> isActorDerived = nullptr) {
 
     REGIONBASEDISOLATION_VERBOSE_LOG(llvm::dbgs() << "Applying: ";
@@ -431,36 +431,37 @@ class Partition {
       break;
     case PartitionOpKind::Transfer: {
       assert(op.OpArgs.size() == 1 &&
-             "Consume PartitionOp should be passed 1 argument");
+             "Transfer PartitionOp should be passed 1 argument");
       assert(labels.count(op.OpArgs[0]) &&
-             "Consume PartitionOp's argument should already be tracked");
+             "Transfer PartitionOp's argument should already be tracked");
 
-      // check if any nonconsumables are transferred here, and handle the
+      // check if any nontransferrables are transferred here, and handle the
       // failure if so
-      for (Element nonconsumable : nonconsumables) {
-        assert(labels.count(nonconsumable) &&
-               "nonconsumables should be function args and self, and therefore"
-               "always present in the label map because of initialization at "
-               "entry");
-        if (!isTransferred(nonconsumable) &&
-            labels.at(nonconsumable) == labels.at(op.OpArgs[0])) {
-          handleConsumeNonConsumable(op, nonconsumable);
+      for (Element nonTransferrable : nontransferrables) {
+        assert(
+            labels.count(nonTransferrable) &&
+            "nontransferrables should be function args and self, and therefore"
+            "always present in the label map because of initialization at "
+            "entry");
+        if (!isTransferred(nonTransferrable) &&
+            labels.at(nonTransferrable) == labels.at(op.OpArgs[0])) {
+          handleTransferNonTransferrable(op, nonTransferrable);
           break;
         }
       }
 
       // If this value is actor derived or if any elements in its region are
-      // actor derived, we need to treat as non-consumable.
+      // actor derived, we need to treat as nontransferrable.
       if (isActorDerived && isActorDerived(op.OpArgs[0]))
-        return handleConsumeNonConsumable(op, op.OpArgs[0]);
+        return handleTransferNonTransferrable(op, op.OpArgs[0]);
       Region elementRegion = labels.at(op.OpArgs[0]);
       if (llvm::any_of(labels,
                        [&](const std::pair<Element, Region> &pair) -> bool {
                          if (pair.second != elementRegion)
                            return false;
                          return isActorDerived && isActorDerived(pair.first);
                        }))
-        return handleConsumeNonConsumable(op, op.OpArgs[0]);
+        return handleTransferNonTransferrable(op, op.OpArgs[0]);
 
       // Ensure if the region is transferred...
       if (!isTransferred(op.OpArgs[0]))
diff --git a/lib/SILOptimizer/Mandatory/TransferNonSendable.cpp b/lib/SILOptimizer/Mandatory/TransferNonSendable.cpp
@@ -482,7 +482,7 @@ class PartitionOpTranslator {
     for (SILArgument *arg : functionArguments) {
       if (auto state = tryToTrackValue(arg)) {
         // If we have an arg that is an actor, we allow for it to be
-        // consumed... value ids derived from it though cannot be consumed.
+        // transfer... value ids derived from it though cannot be transferred.
         LLVM_DEBUG(llvm::dbgs() << "    %%" << state->getID());
         neverTransferredValueIDs.push_back(state->getID());
         nonSendableIndices.push_back(state->getID());
@@ -1071,7 +1071,7 @@ class PartitionOpTranslator {
 
     // We ignore begin_access since we look through them. We look through them
     // since we want to treat the use of the begin_access as the semantic giving
-    // instruction. Otherwise, if we have a store after a consume we will emit
+    // instruction. Otherwise, if we have a store after a transfer we will emit
     // an error on the begin_access rather than allowing for the store to
     // overwrite the original value. This would then cause an error.
     //
@@ -1343,7 +1343,7 @@ struct LocalTransferredReason {
 /// causes a target access to be invalid because of merging/joining that spans
 /// many different blocks worth of control flow, it is less likely to be
 /// informative, so distance is used as a heuristic to choose which access sites
-/// to display in diagnostics given a racy consumption.
+/// to display in diagnostics given a racy transfer.
 class TransferredReason {
   std::multimap<unsigned, PartitionOp> transferOps;
 
@@ -1437,7 +1437,7 @@ class TransferRequireAccumulator {
       unsigned numDisplayed = numProcessed;
       unsigned numHidden = requireOps.size() - numProcessed;
       if (!tryDiagnoseAsCallSite(transferOp, numDisplayed, numHidden)) {
-        assert(false && "no consumptions besides callsites implemented yet");
+        assert(false && "no transfers besides callsites implemented yet");
 
         // Default to a more generic diagnostic if we can't find the callsite.
         auto expr = getExprForPartitionOp(transferOp);
@@ -1486,16 +1486,17 @@ class TransferRequireAccumulator {
     SILInstruction *sourceInst =
         transferOp.getSourceInst(/*assertNonNull=*/true);
     ApplyExpr *apply = sourceInst->getLoc().getAsASTNode<ApplyExpr>();
+
+    // If the transfer does not correspond to an apply expression... bail.
     if (!apply)
-      // consumption does not correspond to an apply expression
       return false;
+
     auto isolationCrossing = apply->getIsolationCrossing();
     assert(isolationCrossing && "ApplyExprs should be transferring only if "
                                 "they are isolation crossing");
 
     auto argExpr = transferOp.getSourceExpr();
-    assert(argExpr &&
-           "sourceExpr should be populated for ApplyExpr consumptions");
+    assert(argExpr && "sourceExpr should be populated for ApplyExpr transfers");
 
     sourceInst->getFunction()
         ->getASTContext()
@@ -1648,22 +1649,22 @@ class RaceTracer {
   findLocalTransferredReason(SILBasicBlock *SILBlock,
                              TrackableValueID transferredVal,
                              std::optional<PartitionOp> targetOp = {}) {
-    // if this is a query for consumption reason at block exit, check the cache
+    // If this is a query for transfer reason at block exit, check the cache.
     if (!targetOp && transferredAtExitReasons.count({SILBlock, transferredVal}))
       return transferredAtExitReasons.at({SILBlock, transferredVal});
 
     const BlockPartitionState &block = blockStates[SILBlock];
 
-    // if targetOp is null, we're checking why the value is transferred at exit,
+    // If targetOp is null, we're checking why the value is transferred at exit,
     // so assert that it's actually transferred at exit
     assert(targetOp || block.getExitPartition().isTransferred(transferredVal));
 
     std::optional<LocalTransferredReason> transferredReason;
 
     Partition workingPartition = block.getEntryPartition();
 
-    // we're looking for a local reason, so if the value is transferred at
-    // entry, revive it for the sake of this search
+    // We are looking for a local reason, so if the value is transferred at
+    // entry, revive it for the sake of this search.
     if (workingPartition.isTransferred(transferredVal))
       workingPartition.apply(PartitionOp::AssignFresh(transferredVal));
 
@@ -1674,35 +1675,35 @@ class RaceTracer {
       workingPartition.apply(partitionOp);
       if (workingPartition.isTransferred(transferredVal) &&
           !transferredReason) {
-        // this partitionOp transfers the target value
+        // This partitionOp transfers the target value.
         if (partitionOp.getKind() == PartitionOpKind::Transfer)
           transferredReason = LocalTransferredReason::TransferInst(partitionOp);
         else
-          // a merge or assignment invalidated this, but that will be a separate
-          // failure to diagnose, so we don't worry about it here
+          // A merge or assignment invalidated this, but that will be a separate
+          // failure to diagnose, so we don't worry about it here.
           transferredReason = LocalTransferredReason::NonTransferInst();
       }
       if (!workingPartition.isTransferred(transferredVal) && transferredReason)
-        // value is no longer transferred - e.g. reassigned or assigned fresh
+        // Value is no longer transferred - e.g. reassigned or assigned fresh.
         transferredReason = llvm::None;
 
       // continue walking block
       i++;
       return true;
     });
 
-    // if we failed to find a local transfer reason, but the value was
-    // transferred at entry to the block, then the reason is "NonLocal"
+    // If we failed to find a local transfer reason, but the value was
+    // transferred at entry to the block, then the reason is "NonLocal".
     if (!transferredReason &&
         block.getEntryPartition().isTransferred(transferredVal))
       transferredReason = LocalTransferredReason::NonLocal();
 
-    // if transferredReason is none, then transferredVal was not actually
-    // transferred
+    // If transferredReason is none, then transferredVal was not actually
+    // transferred.
     assert(transferredReason || dumpBlockSearch(SILBlock, transferredVal) &&
-                                    " no consumption was found");
+                                    " no transfer was found");
 
-    // if this is a query for consumption reason at block exit, update the cache
+    // If this is a query for a transfer reason at block exit, update the cache.
     if (!targetOp)
       return transferredAtExitReasons[std::pair{SILBlock, transferredVal}] =
                  transferredReason.value();
@@ -1908,7 +1909,7 @@ class PartitionAnalysis {
           [&](const PartitionOp &partitionOp, TrackableValueID transferredVal) {
             auto expr = getExprForPartitionOp(partitionOp);
 
-            // ensure that multiple consumptions at the same AST node are only
+            // ensure that multiple transfers at the same AST node are only
             // entered once into the race tracer
             if (hasBeenEmitted(expr))
               return;
@@ -1923,10 +1924,10 @@ class PartitionAnalysis {
             raceTracer.traceUseOfTransferredValue(partitionOp, transferredVal);
           },
 
-          /*handleTransferNonConsumable=*/
+          /*handleTransferNonTransferrable=*/
           [&](const PartitionOp &partitionOp, TrackableValueID transferredVal) {
             LLVM_DEBUG(llvm::dbgs()
-                       << "Emitting ConsumeNonConsume Error!\n"
+                       << "Emitting TransferNonTransferrable Error!\n"
                        << "ID:  %%" << transferredVal << "\n"
                        << "Rep: "
                        << *translator.getValueForId(transferredVal)
@@ -1940,7 +1941,7 @@ class PartitionAnalysis {
     LLVM_DEBUG(llvm::dbgs() << "Accumulator Complete:\n";
                raceTracer.getAccumulator().print(llvm::dbgs()););
 
-    // Ask the raceTracer to report diagnostics at the consumption sites for all
+    // Ask the raceTracer to report diagnostics at the transfer sites for all
     // the racy requirement sites entered into it above.
     raceTracer.getAccumulator().emitErrorsForTransferRequire(
         NUM_REQUIREMENTS_TO_DIAGNOSE);
@@ -1951,9 +1952,12 @@ class PartitionAnalysis {
     SILInstruction *sourceInst =
         transferOp.getSourceInst(/*assertNonNull=*/true);
     ApplyExpr *apply = sourceInst->getLoc().getAsASTNode<ApplyExpr>();
+
+    // If the transfer does not correspond to an apply expression... return
+    // early.
     if (!apply)
-      // consumption does not correspond to an apply expression
       return false;
+
     auto isolationCrossing = apply->getIsolationCrossing();
     if (!isolationCrossing) {
       assert(false && "ApplyExprs should be transferring only if"
@@ -1962,8 +1966,7 @@ class PartitionAnalysis {
     }
     auto argExpr = transferOp.getSourceExpr();
     if (!argExpr)
-      assert(false &&
-             "sourceExpr should be populated for ApplyExpr consumptions");
+      assert(false && "sourceExpr should be populated for ApplyExpr transfers");
 
     function->getASTContext()
         .Diags
