[region-isolation] Learn how to find variable names around phi arguments.

With this, the only remaining "task or actor isolated" error is due to changes I
need to land around async let.

Author: gottesmm

include/swift/SILOptimizer/Utils/VariableNameUtils.h

@@ -17,6 +17,7 @@
 #ifndef SWIFT_SILOPTIMIZER_UTILS_VARIABLENAMEUTILS_H
 #define SWIFT_SILOPTIMIZER_UTILS_VARIABLENAMEUTILS_H
 
+#include "swift/Basic/Defer.h"
 #include "swift/Basic/OptionSet.h"
 #include "swift/SIL/ApplySite.h"
 #include "swift/SIL/DebugUtils.h"
@@ -40,8 +41,76 @@ class VariableNameInferrer {
   using Options = OptionSet<Flag>;
 
 private:
-  /// The stacklist that we use to process from use->
-  StackList<PointerUnion<SILInstruction *, SILValue>> variableNamePath;
+  template <typename T, unsigned smallSize>
+  class VariableNamePathArray {
+    SmallVector<T, smallSize> data;
+
+    unsigned lastSnapShotIndex = 0;
+    unsigned insertionPointIndex = 0;
+
+  public:
+    VariableNamePathArray() : data() {}
+
+    ArrayRef<T> getData() const {
+      assert(insertionPointIndex <= data.size());
+      return ArrayRef<T>(data).take_front(insertionPointIndex);
+    }
+
+    void print(llvm::raw_ostream &os) const {
+      os << "LastSnapShotIndex: " << lastSnapShotIndex << '\n';
+      os << "InsertionPointIndex: " << insertionPointIndex << '\n';
+    }
+
+    SWIFT_DEBUG_DUMP { print(llvm::dbgs()); }
+
+    /// Pushes a snapshot and returns the old index.
+    unsigned pushSnapShot() & {
+      // After we run with a snapshot, we want:
+      //
+      // 1. lastSnapShotIndex to return to its value before the continuation
+      // ran.
+      // 2. The insertion point index becomes lastSnapShotIndex.
+      unsigned oldSnapShotIndex = lastSnapShotIndex;
+      lastSnapShotIndex = insertionPointIndex;
+      return oldSnapShotIndex;
+    }
+
+    void popSnapShot(unsigned oldIndex) & {
+      insertionPointIndex = lastSnapShotIndex;
+      lastSnapShotIndex = oldIndex;
+    }
+
+    void returnSnapShot(unsigned oldIndex) & { lastSnapShotIndex = oldIndex; }
+
+    void push_back(const T &newValue) & {
+      SWIFT_DEFER { assert(insertionPointIndex <= data.size()); };
+      if (insertionPointIndex == data.size()) {
+        data.push_back(newValue);
+        ++insertionPointIndex;
+        return;
+      }
+
+      data[insertionPointIndex] = newValue;
+      ++insertionPointIndex;
+    }
+
+    [[nodiscard]] T pop_back_val() & {
+      SWIFT_DEFER { assert(insertionPointIndex <= data.size()); };
+      assert(!lastSnapShotIndex &&
+             "Can only pop while lastSnapShotIndex is not set");
+      --insertionPointIndex;
+      return data[insertionPointIndex];
+    }
+
+    bool empty() const { return !insertionPointIndex; }
+  };
+
+  /// The stacklist that we use to print out variable names.
+  ///
+  /// Has to be a small vector since we push/pop the last segment start. This
+  /// lets us speculate when processing phis.
+  VariableNamePathArray<PointerUnion<SILInstruction *, SILValue>, 4>
+      variableNamePath;
 
   /// The root value of our string.
   ///
@@ -59,12 +128,12 @@ class VariableNameInferrer {
 
 public:
   VariableNameInferrer(SILFunction *fn, SmallString<64> &resultingString)
-      : variableNamePath(fn), resultingString(resultingString) {}
+      : variableNamePath(), resultingString(resultingString) {}
 
   VariableNameInferrer(SILFunction *fn, Options options,
                        SmallString<64> &resultingString)
-      : variableNamePath(fn), resultingString(resultingString),
-        options(options) {}
+      : variableNamePath(), resultingString(resultingString), options(options) {
+  }
 
   /// Attempts to infer a name from just uses of \p searchValue.
   ///
@@ -133,7 +202,12 @@ class VariableNameInferrer {
 
   /// Do not call this directly. Used just to improve logging for
   /// findDebugInfoProvidingValue.
-  SILValue findDebugInfoProvidingValueHelper(SILValue searchValue);
+  SILValue findDebugInfoProvidingValueHelper(SILValue searchValue,
+                                             ValueSet &visitedValues);
+
+  /// A special helper for handling phi values. Do not call this directly.
+  SILValue findDebugInfoProvidingValuePhiArg(SILValue incomingValue,
+                                             ValueSet &visitedValues);
 
   /// Given an initialized once allocation inst without a ValueDecl or a
   /// DebugVariable provided name, attempt to find a root value from its

lib/SILOptimizer/Utils/VariableNameUtils.cpp

@@ -214,7 +214,8 @@ VariableNameInferrer::findDebugInfoProvidingValue(SILValue searchValue) {
     return SILValue();
   LLVM_DEBUG(llvm::dbgs() << "Searching for debug info providing value for: "
                           << searchValue);
-  SILValue result = findDebugInfoProvidingValueHelper(searchValue);
+  ValueSet valueSet(searchValue->getFunction());
+  SILValue result = findDebugInfoProvidingValueHelper(searchValue, valueSet);
   if (result) {
     LLVM_DEBUG(llvm::dbgs() << "Result: " << result);
   } else {
@@ -223,12 +224,45 @@ VariableNameInferrer::findDebugInfoProvidingValue(SILValue searchValue) {
   return result;
 }
 
-SILValue
-VariableNameInferrer::findDebugInfoProvidingValueHelper(SILValue searchValue) {
+SILValue VariableNameInferrer::findDebugInfoProvidingValuePhiArg(
+    SILValue incomingValue, ValueSet &visitedValues) {
+  // We use pushSnapShot to run recursively and if we fail to find a
+  // value, we just pop our list to the last snapshot end of list. If we
+  // succeed, we do not pop and just return recusive value. Our user
+  // will consume variableNamePath at this point.
+  LLVM_DEBUG(llvm::dbgs() << "Before pushing a snap shot!\n";
+             variableNamePath.print(llvm::dbgs()));
+
+  unsigned oldSnapShotIndex = variableNamePath.pushSnapShot();
+  LLVM_DEBUG(llvm::dbgs() << "After pushing a snap shot!\n";
+             variableNamePath.print(llvm::dbgs()));
+
+  if (SILValue recursiveValue =
+          findDebugInfoProvidingValueHelper(incomingValue, visitedValues)) {
+    LLVM_DEBUG(llvm::dbgs() << "Returned: " << recursiveValue);
+    variableNamePath.returnSnapShot(oldSnapShotIndex);
+    return recursiveValue;
+  }
+
+  variableNamePath.popSnapShot(oldSnapShotIndex);
+  LLVM_DEBUG(llvm::dbgs() << "After popping a snap shot!\n";
+             variableNamePath.print(llvm::dbgs()));
+  return SILValue();
+}
+
+SILValue VariableNameInferrer::findDebugInfoProvidingValueHelper(
+    SILValue searchValue, ValueSet &visitedValues) {
   assert(searchValue);
 
   while (true) {
     assert(searchValue);
+
+    // If we already visited the value, return SILValue(). This prevents issues
+    // caused by looping phis. We treat this as a failure and visit the either
+    // phi values.
+    if (!visitedValues.insert(searchValue))
+      return SILValue();
+
     LLVM_DEBUG(llvm::dbgs() << "Value: " << *searchValue);
 
     if (auto *allocInst = dyn_cast<AllocationInst>(searchValue)) {
@@ -306,6 +340,17 @@ VariableNameInferrer::findDebugInfoProvidingValueHelper(SILValue searchValue) {
       continue;
     }
 
+    // Enums only have a single possible parent and is used sometimes like a
+    // transformation (e.x.: constructing an optional). We want to look through
+    // them and add the case to the variableNamePath.
+    if (auto *e = dyn_cast<EnumInst>(searchValue)) {
+      if (e->hasOperand()) {
+        variableNamePath.push_back(e);
+        searchValue = e->getOperand();
+        continue;
+      }
+    }
+
     if (auto *dti = dyn_cast_or_null<DestructureTupleInst>(
             searchValue->getDefiningInstruction())) {
       // Append searchValue, so we can find the specific tuple index.
@@ -329,6 +374,27 @@ VariableNameInferrer::findDebugInfoProvidingValueHelper(SILValue searchValue) {
       }
     }
 
+    // If we have a phi argument, visit each of the incoming values and pick the
+    // first one that gives us a name.
+    if (auto *phiArg = dyn_cast<SILPhiArgument>(searchValue)) {
+      if (auto *term = phiArg->getSingleTerminator()) {
+        if (auto *swi = dyn_cast<SwitchEnumInst>(term)) {
+          if (auto value = findDebugInfoProvidingValuePhiArg(swi->getOperand(),
+                                                             visitedValues))
+            return value;
+        }
+      }
+
+      SmallVector<SILValue, 8> incomingValues;
+      if (phiArg->getIncomingPhiValues(incomingValues)) {
+        for (auto value : incomingValues) {
+          if (auto resultValue =
+                  findDebugInfoProvidingValuePhiArg(value, visitedValues))
+            return resultValue;
+        }
+      }
+    }
+
     auto getNamePathComponentFromCallee = [&](FullApplySite call) -> SILValue {
       // Use the name of the property being accessed if we can get to it.
       if (isa<FunctionRefBaseInst>(call.getCallee()) ||
@@ -506,6 +572,11 @@ void VariableNameInferrer::popSingleVariableName() {
       return;
     }
 
+    if (auto *ei = dyn_cast<EnumInst>(inst)) {
+      resultingString += getNameFromDecl(ei->getElement());
+      return;
+    }
+
     resultingString += "<unknown decl>";
     return;
   }

test/Concurrency/transfernonsendable.sil

@@ -277,7 +277,8 @@ bb2(%1 : @guaranteed $NonSendableKlass):
   br bb3(%3 : $FakeOptional<NonSendableKlass>)
 
 bb3(%4 : @owned $FakeOptional<NonSendableKlass>):
-  return %4 : $FakeOptional<NonSendableKlass> // expected-warning {{task or actor isolated value cannot be transferred}}
+  return %4 : $FakeOptional<NonSendableKlass> // expected-warning {{transferring 'myname.some' may cause a race}}
+  // expected-note @-1 {{main actor-isolated 'myname.some' cannot be a transferring result. main actor-isolated uses may race with caller uses}}
 }
 
 sil [ossa] @warningIfCallingGetter : $@convention(method) @async (@sil_isolated @guaranteed MyActor) -> () {

test/Concurrency/transfernonsendable_strong_transferring_results.swift

@@ -153,8 +153,8 @@ extension MainActorIsolatedEnum {
     case .second(let ns):
       return ns
     }
-    // TODO: This should be on return ns.
-  } // expected-warning {{task or actor isolated value cannot be transferred}}
+  } // expected-warning {{transferring 'ns.some' may cause a race}}
+  // expected-note @-1 {{main actor-isolated 'ns.some' cannot be a transferring result. main actor-isolated uses may race with caller uses}}
 
   func testSwitchReturnNoTransfer() -> NonSendableKlass? {
     switch self {
@@ -170,7 +170,8 @@ extension MainActorIsolatedEnum {
       return ns // TODO: The error below should be here.
     }
     return nil
-  } // expected-warning {{task or actor isolated value cannot be transferred}} 
+  } // expected-warning {{transferring 'ns.some' may cause a race}}
+  // expected-note @-1 {{main actor-isolated 'ns.some' cannot be a transferring result. main actor-isolated uses may race with caller uses}} 
 
   func testIfLetReturnNoTransfer() -> NonSendableKlass? {
     if case .second(let ns) = self {

test/SILOptimizer/moveonly_borrowing_switch.swift

@@ -214,15 +214,15 @@ func testOuterAO(consuming bas: consuming AOBas) { // expected-error{{'bas' used
       where condition(x):
         nibble(payload: x)
         eat(payload: x)
-    case .payload(let x) // expected-error {{'unknown' is borrowed and cannot be consumed}}
+    case .payload(let x) // expected-error {{'bas.payload' is borrowed and cannot be consumed}}
       where hungryCondition(x): // expected-note {{consumed here}}
         nibble(payload: x)
         eat(payload: x)
     case .payload(.loadablePayload(let x))
       where condition(x):
         nibble(payload: x)
         eat(payload: x)
-    case .payload(.loadablePayload(.payload(let x))) // expected-error{{'unknown' is borrowed and cannot be consumed}}
+    case .payload(.loadablePayload(.payload(let x))) // expected-error{{'bas.payload.loadablePayload' is borrowed and cannot be consumed}}
       where hungryCondition(x): // expected-note{{consumed here}}
         nibble(payload: x)
         eat(payload: x)