[move-only-address] Now that we borrow move only types when we call functions, enable the main move only address transform.

The main constraint that we needed to fight here is that we are not allowed in
the move only address checker to expand exclusivity scopes since we run after
the main exclusivity passes. This means that to enable this transformation I
needed to add a new error diagnostic that is emitted if we are unable to convert
a loadCopyOrTake operation to a borrow like operation due to an exclusivity
scope that is too tight. Example:

```
%0 = begin_access [read] %addr
%1 = load [copy] %0
end_access %0
...
destroy_value %1
```

Luckily this does not actually happen for move only structs/move only enums
after 525ba8c which makes it so that we perform
a true borrow of those types when passing them to functions. If we pass a move
only class directly, we get the same behavior. Sadly if we pass a field from a
move only class, we still get a tight exclusivity scope and thus will emit an
error today. This is an issue that we can fight at a later time once we decide
to productize move only classes further. But at least structs/enums work
well. One can see this behavior by looking at all of the places that needed to
be updated as a result of this change.

rdar://102491093

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -735,6 +735,8 @@ ERROR(sil_moveonlychecker_inout_not_reinitialized_before_end_of_function, none,
       "'%0' consumed but not reinitialized before end of function", (StringRef))
 ERROR(sil_moveonlychecker_value_consumed_in_a_loop, none,
       "'%0' consumed by a use in a loop", (StringRef))
+ERROR(sil_moveonlychecker_exclusivity_violation, none,
+      "'%0' has consuming use that cannot be eliminated due to a tight exclusivity scope", (StringRef))
 
 NOTE(sil_moveonlychecker_consuming_use_here, none,
      "consuming use", ())

lib/SILOptimizer/Mandatory/MoveOnlyAddressChecker.cpp

@@ -146,6 +146,7 @@
 #include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/SmallBitVector.h"
 #include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 
@@ -204,10 +205,31 @@ struct DiagnosticEmitter {
                                         SILInstruction *violatingUse);
   void emitAddressDiagnosticNoCopy(MarkMustCheckInst *markedValue,
                                    SILInstruction *consumingUse);
+  void emitExclusivityHazardDiagnostic(MarkMustCheckInst *markedValue,
+                                       SILInstruction *consumingUse);
 };
 
 } // namespace
 
+void DiagnosticEmitter::emitExclusivityHazardDiagnostic(
+    MarkMustCheckInst *markedValue, SILInstruction *consumingUse) {
+  if (!useWithDiagnostic.insert(consumingUse).second)
+    return;
+
+  auto &astContext = markedValue->getFunction()->getASTContext();
+  StringRef varName = getVariableNameForValue(markedValue);
+
+  LLVM_DEBUG(llvm::dbgs() << "Emitting error for exclusivity!\n");
+  LLVM_DEBUG(llvm::dbgs() << "    Mark: " << *markedValue);
+  LLVM_DEBUG(llvm::dbgs() << "    Consuming use: " << *consumingUse);
+
+  diagnose(astContext,
+           markedValue->getDefiningInstruction()->getLoc().getSourceLoc(),
+           diag::sil_moveonlychecker_exclusivity_violation, varName);
+  diagnose(astContext, consumingUse->getLoc().getSourceLoc(),
+           diag::sil_moveonlychecker_consuming_use_here);
+}
+
 void DiagnosticEmitter::emitAddressDiagnostic(
     MarkMustCheckInst *markedValue, SILInstruction *lastLiveUse,
     SILInstruction *violatingUse, bool isUseConsuming,
@@ -775,13 +797,18 @@ struct GatherUsesVisitor : public AccessUseVisitor {
   bool emittedEarlyDiagnostic = false;
   DiagnosticEmitter &diagnosticEmitter;
 
+  // Pruned liveness used to validate that load [take]/load [copy] can be
+  // converted to load_borrow without violating exclusivity.
+  SSAPrunedLiveness &liveness;
+
   GatherUsesVisitor(MoveOnlyChecker &moveChecker, UseState &useState,
                     MarkMustCheckInst *markedValue,
-                    DiagnosticEmitter &diagnosticEmitter)
+                    DiagnosticEmitter &diagnosticEmitter,
+                    SSAPrunedLiveness &gatherUsesLiveness)
       : AccessUseVisitor(AccessUseType::Overlapping,
                          NestedAccessType::IgnoreAccessBegin),
         moveChecker(moveChecker), useState(useState), markedValue(markedValue),
-        diagnosticEmitter(diagnosticEmitter) {}
+        diagnosticEmitter(diagnosticEmitter), liveness(gatherUsesLiveness) {}
 
   bool visitUse(Operand *op, AccessUseType useTy) override;
   void reset(SILValue address) { useState.address = address; }
@@ -795,6 +822,41 @@ struct GatherUsesVisitor : public AccessUseVisitor {
   /// base address that we are checking which should be the operand of the mark
   /// must check value.
   SILValue getRootAddress() const { return markedValue; }
+
+  /// Returns true if we emitted an error.
+  bool checkForExclusivityHazards(LoadInst *li) {
+    SWIFT_DEFER { liveness.clear(); };
+
+    LLVM_DEBUG(llvm::dbgs() << "Checking for exclusivity hazards for: " << *li);
+
+    // Grab our access path with in scope. We want to find the inner most access
+    // scope.
+    auto accessPathWithBase =
+        AccessPathWithBase::computeInScope(li->getOperand());
+    auto accessPath = accessPathWithBase.accessPath;
+    // TODO: Make this a we don't understand error.
+    assert(accessPath.isValid() && "Invalid access path?!");
+
+    auto *bai = dyn_cast<BeginAccessInst>(accessPathWithBase.base);
+
+    if (!bai) {
+      LLVM_DEBUG(llvm::dbgs()
+                 << "    No begin access... so no exclusivity violation!\n");
+      return false;
+    }
+
+    bool emittedError = false;
+    liveness.initializeDef(bai);
+    liveness.computeSimple();
+    for (auto *consumingUse : li->getConsumingUses()) {
+      if (!liveness.isWithinBoundary(consumingUse->getUser())) {
+        diagnosticEmitter.emitExclusivityHazardDiagnostic(
+            markedValue, consumingUse->getUser());
+        emittedError = true;
+      }
+    }
+    return emittedError;
+  }
 };
 
 } // end anonymous namespace
@@ -934,6 +996,14 @@ bool GatherUsesVisitor::visitUse(Operand *op, AccessUseType useTy) {
           return false;
 
         LLVM_DEBUG(llvm::dbgs() << "Found potential borrow: " << *user);
+
+        if (checkForExclusivityHazards(li)) {
+          LLVM_DEBUG(llvm::dbgs() << "Found exclusivity violation?!\n");
+          emittedEarlyDiagnostic = true;
+          moveChecker.valuesWithDiagnostics.insert(markedValue);
+          return true;
+        }
+
         useState.borrows.insert({user, *leafRange});
         return true;
       }
@@ -965,7 +1035,14 @@ bool GatherUsesVisitor::visitUse(Operand *op, AccessUseType useTy) {
         return false;
 
       if (moveChecker.finalConsumingUses.empty()) {
-        LLVM_DEBUG(llvm::dbgs() << "Found borrow inst: " << *user);
+        LLVM_DEBUG(llvm::dbgs() << "Found potential borrow inst: " << *user);
+        if (checkForExclusivityHazards(li)) {
+          LLVM_DEBUG(llvm::dbgs() << "Found exclusivity violation?!\n");
+          emittedEarlyDiagnostic = true;
+          moveChecker.valuesWithDiagnostics.insert(markedValue);
+          return true;
+        }
+
         useState.borrows.insert({user, *leafRange});
       } else {
         LLVM_DEBUG(llvm::dbgs() << "Found take inst: " << *user);
@@ -1155,8 +1232,10 @@ bool MoveOnlyChecker::performSingleCheck(MarkMustCheckInst *markedAddress) {
   // Then gather all uses of our address by walking from def->uses. We use this
   // to categorize the uses of this address into their ownership behavior (e.x.:
   // init, reinit, take, destroy, etc.).
+  SmallVector<SILBasicBlock *, 32> gatherUsesDiscoveredBlocks;
+  SSAPrunedLiveness gatherUsesLiveness(&gatherUsesDiscoveredBlocks);
   GatherUsesVisitor visitor(*this, addressUseState, markedAddress,
-                            diagnosticEmitter);
+                            diagnosticEmitter, gatherUsesLiveness);
   SWIFT_DEFER { visitor.clear(); };
   visitor.reset(markedAddress);
   if (!visitAccessPathUses(visitor, accessPath, fn)) {
@@ -1467,13 +1546,6 @@ bool MoveOnlyChecker::performSingleCheck(MarkMustCheckInst *markedAddress) {
     }
   }
 
-  // Now before we do anything more, just exit and say we errored. This will not
-  // actually make us fail and will cause code later in the checker to rewrite
-  // all non-explicit copy instructions to their explicit variant. I did this
-  // for testing purposes only.
-  valuesWithDiagnostics.insert(markedAddress);
-  return true;
-
   // Ok, we not have emitted our main errors. Now we begin the
   // transformation. We begin by processing borrows. We can also emit errors
   // here if we find that we can not expand the borrow scope of the load [copy]
@@ -1487,50 +1559,16 @@ bool MoveOnlyChecker::performSingleCheck(MarkMustCheckInst *markedAddress) {
         // must have been destroy_value. So we can just gather up those
         // destroy_value and use then to create a new load_borrow scope.
         SILBuilderWithScope builder(li);
-        SILValue base = stripAccessMarkers(li->getOperand());
-        bool foundAccess = base != li->getOperand();
-
-        // Insert a new access scope.
-        SILValue newBase = base;
-        if (foundAccess) {
-          newBase = builder.createBeginAccess(
-              li->getLoc(), base, SILAccessKind::Read,
-              SILAccessEnforcement::Static, false /*no nested conflict*/,
-              false /*is from builtin*/);
-        }
-        auto *lbi = builder.createLoadBorrow(li->getLoc(), newBase);
+        auto *lbi = builder.createLoadBorrow(li->getLoc(), li->getOperand());
 
         for (auto *consumeUse : li->getConsumingUses()) {
           auto *dvi = cast<DestroyValueInst>(consumeUse->getUser());
           SILBuilderWithScope destroyBuilder(dvi);
           destroyBuilder.createEndBorrow(dvi->getLoc(), lbi);
-          if (foundAccess)
-            destroyBuilder.createEndAccess(dvi->getLoc(), newBase,
-                                           false /*aborted*/);
           destroys.push_back(dvi);
           changed = true;
         }
 
-        // TODO: We need to check if this is the only use.
-        if (auto *access = dyn_cast<BeginAccessInst>(li->getOperand())) {
-          bool foundUnknownUse = false;
-          for (auto *accUse : access->getUses()) {
-            if (accUse->getUser() == li)
-              continue;
-            if (auto *ea = dyn_cast<EndAccessInst>(accUse->getUser())) {
-              endAccesses.push_back(ea);
-              continue;
-            }
-            foundUnknownUse = true;
-          }
-          if (!foundUnknownUse) {
-            for (auto *end : endAccesses)
-              end->eraseFromParent();
-            access->replaceAllUsesWithUndef();
-            access->eraseFromParent();
-          }
-        }
-
         for (auto *d : destroys)
           d->eraseFromParent();
         li->replaceAllUsesWith(lbi);
@@ -1539,6 +1577,7 @@ bool MoveOnlyChecker::performSingleCheck(MarkMustCheckInst *markedAddress) {
       }
     }
 
+    llvm::dbgs() << "Borrow: " << *pair.first;
     llvm_unreachable("Unhandled case?!");
   }
 
@@ -1570,6 +1609,16 @@ bool MoveOnlyChecker::performSingleCheck(MarkMustCheckInst *markedAddress) {
             }
             llvm_unreachable("Error?! This is a double destroy?!");
           }
+
+          if (auto *copyAddr = dyn_cast<CopyAddrInst>(&*ii)) {
+            if (!copyAddr->isTakeOfSrc()) {
+              destroy->eraseFromParent();
+              changed = true;
+              foundSingleBlockCase = true;
+              break;
+            }
+            llvm_unreachable("Error?! This is a double destroy?!");
+          }
         }
 
         if (addressUseState.livenessUses.count(&*ii)) {
@@ -1606,6 +1655,17 @@ bool MoveOnlyChecker::performSingleCheck(MarkMustCheckInst *markedAddress) {
       }
     }
 
+    if (auto *copy = dyn_cast<CopyAddrInst>(take.first)) {
+      if (copy->isTakeOfSrc())
+        continue;
+      // Convert this to its take form.
+      if (auto *access = dyn_cast<BeginAccessInst>(copy->getSrc()))
+        access->setAccessKind(SILAccessKind::Modify);
+      copy->setIsTakeOfSrc(IsTake);
+      continue;
+    }
+
+    llvm::dbgs() << "Take User: " << *take.first;
     llvm_unreachable("Unhandled case?!");
   }
 

test/SILOptimizer/moveonly_addresschecker_diagnostics.swift

@@ -339,35 +339,43 @@ public func classAssignToVar5Arg2(_ x: Klass, _ x2: inout Klass) { // expected-e
 
 public func classAccessAccessField(_ x: Klass) { // expected-error {{'x' has guaranteed ownership but was consumed}}
     var x2 = x // expected-note {{consuming use}}
+    // expected-error @-1 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
+    // expected-error @-2 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
     x2 = Klass()
-    classUseMoveOnlyWithoutEscaping(x2.k!)
+    classUseMoveOnlyWithoutEscaping(x2.k!) // expected-note {{consuming use}}
     for _ in 0..<1024 {
-        classUseMoveOnlyWithoutEscaping(x2.k!)
+        classUseMoveOnlyWithoutEscaping(x2.k!) // expected-note {{consuming use}}
     }
 }
 
 public func classAccessAccessFieldArg(_ x2: inout Klass) {
-    classUseMoveOnlyWithoutEscaping(x2.k!)
+    // expected-error @-1 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
+    // expected-error @-2 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
+    classUseMoveOnlyWithoutEscaping(x2.k!) // expected-note {{consuming use}}
     for _ in 0..<1024 {
-        classUseMoveOnlyWithoutEscaping(x2.k!)
+        classUseMoveOnlyWithoutEscaping(x2.k!) // expected-note {{consuming use}}
     }
 }
 
 public func classAccessConsumeField(_ x: Klass) { // expected-error {{'x' has guaranteed ownership but was consumed}}
     var x2 = x // expected-note {{consuming use}}
+    // expected-error @-1 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
+    // expected-error @-2 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
     x2 = Klass()
     // Since a class is a reference type, we do not emit an error here.
-    classConsume(x2.k!)
+    classConsume(x2.k!) // expected-note {{consuming use}}
     for _ in 0..<1024 {
-        classConsume(x2.k!)
+        classConsume(x2.k!) // expected-note {{consuming use}}
     }
 }
 
 public func classAccessConsumeFieldArg(_ x2: inout Klass) {
+    // expected-error @-1 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
+    // expected-error @-2 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
     // Since a class is a reference type, we do not emit an error here.
-    classConsume(x2.k!)
+    classConsume(x2.k!) // expected-note {{consuming use}}
     for _ in 0..<1024 {
-        classConsume(x2.k!)
+        classConsume(x2.k!) // expected-note {{consuming use}}
     }
 }
 
@@ -628,34 +636,42 @@ public func finalClassAssignToVar5Arg(_ x2: inout FinalKlass) { // expected-erro
 
 public func finalClassAccessField() {
     var x2 = FinalKlass()
+    // expected-error @-1 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
+    // expected-error @-2 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
     x2 = FinalKlass()
-    classUseMoveOnlyWithoutEscaping(x2.k!)
+    classUseMoveOnlyWithoutEscaping(x2.k!) // expected-note {{consuming use}}
     for _ in 0..<1024 {
-        classUseMoveOnlyWithoutEscaping(x2.k!)
+        classUseMoveOnlyWithoutEscaping(x2.k!) // expected-note {{consuming use}}
     }
 }
 
 public func finalClassAccessFieldArg(_ x2: inout FinalKlass) {
-    classUseMoveOnlyWithoutEscaping(x2.k!)
+    // expected-error @-1 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
+    // expected-error @-2 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
+    classUseMoveOnlyWithoutEscaping(x2.k!) // expected-note {{consuming use}}
     for _ in 0..<1024 {
-        classUseMoveOnlyWithoutEscaping(x2.k!)
+        classUseMoveOnlyWithoutEscaping(x2.k!) // expected-note {{consuming use}}
     }
 }
 
 public func finalClassConsumeField() {
     var x2 = FinalKlass()
+    // expected-error @-1 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
+    // expected-error @-2 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
     x2 = FinalKlass()
 
-    classConsume(x2.k!)
+    classConsume(x2.k!) // expected-note {{consuming use}}
     for _ in 0..<1024 {
-        classConsume(x2.k!)
+        classConsume(x2.k!) // expected-note {{consuming use}}
     }
 }
 
 public func finalClassConsumeFieldArg(_ x2: inout FinalKlass) {
-    classConsume(x2.k!)
+    // expected-error @-1 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
+    // expected-error @-2 {{'x2' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
+    classConsume(x2.k!) // expected-note {{consuming use}}
     for _ in 0..<1024 {
-        classConsume(x2.k!)
+        classConsume(x2.k!) // expected-note {{consuming use}}
     }
 }
 

test/SILOptimizer/moveonly_deinits.swift

@@ -32,11 +32,12 @@ enum MoveOnlyEnum {
     deinit { // expected-error {{'self' consumed more than once}}
         let x = self // expected-note {{consuming use}}
         _ = x
-        var y = MoveOnlyEnum.lhs(Klass())
+        var y = MoveOnlyEnum.lhs(Klass()) // expected-error {{'y' has consuming use that cannot be eliminated due to a tight exclusivity scope}}
         y = self // expected-note {{consuming use}}
         // We get an infinite recursion since we are going to call our own
         // deinit here. We are just testing diagnostics here though.
         _ = y // expected-warning {{function call causes an infinite recursion}}
+        // expected-note @-1 {{consuming use}}
         globalMoveOnlyEnum = self // expected-note {{consuming use}}
     } // expected-note {{consuming use}}
 }