[move-function] Allow for move to be applied to inout function arguments.

gottesmm · gottesmm · commit e6faa3048855 · 2021-12-13T10:41:20.000-08:00
These are verified like a var except that the parameter will have an implicit
read use on all function exit terminators. This ensures that if a programmer
moves a value out of the inout parameter, a new value must be assigned before
function exit. This is important since otherwise the convention would be
invalidated. Example:

```
func f(_ x: inout T) {
     // Move a value out of x. x does not have a value within it anymore.
     let value = _move(x)
} // So we emit an error saying there is a use here since an inout must have
  // a valid object within it upon function exit due to convention guarantees.
```

As an added side-effect of this, one can now use move as on self in mutating
contexts. Thus one can move the self value out of the self inout binding using
_move and if one does not replace self with a new value by end of function, you
will get a compile time error, e.x.:

```
struct S {
  var buffer: Klass

  mutating func doSomething() {
    let b = move(self).buffer
    // ... do some stuff, maybe get a different buffer ...
    let maybeDifferentBuffer = maybeNewBuffer(b)

    // If we do not re-initialize S with a new value by uncommenting
    // the following line, we will get a compile time error.
    // self = S(differentBuffer)
  }
}
```
diff --git a/lib/SILOptimizer/Mandatory/MoveFunctionCanonicalization.cpp b/lib/SILOptimizer/Mandatory/MoveFunctionCanonicalization.cpp
@@ -112,14 +112,18 @@ tryHandlingLoadableVarMovePattern(MarkUnresolvedMoveAddrInst *markMoveAddr,
 
   LLVM_DEBUG(llvm::dbgs() << "Found LI: " << *li);
   SILValue operand = stripAccessMarkers(li->getOperand());
-  auto *originalASI = dyn_cast<AllocStackInst>(operand);
-  // Make sure that we have a lexical alloc_stack marked as a var or a let. We
-  // don't want properties.
-  if (!originalASI || !originalASI->isLexical() ||
-      !(originalASI->isVar() || originalASI->isLet()))
-    return false;
+  if (auto *originalASI = dyn_cast<AllocStackInst>(operand)) {
+    if (!originalASI->isLexical() ||
+        !(originalASI->isVar() || originalASI->isLet()))
+      return false;
+    LLVM_DEBUG(llvm::dbgs() << "Found OriginalASI: " << *originalASI);
+  } else {
+    auto *fArg = dyn_cast<SILFunctionArgument>(operand);
+    if (!fArg || !fArg->hasConvention(SILArgumentConvention::Indirect_Inout))
+      return false;
+    LLVM_DEBUG(llvm::dbgs() << "Found fArg: " << *fArg);
+  }
 
-  LLVM_DEBUG(llvm::dbgs() << "Found OriginalASI: " << *originalASI);
   // Make sure that there aren't any side-effect having instructions in
   // between our load/store.
   LLVM_DEBUG(llvm::dbgs() << "Checking for uses in between LI and SI.\n");
@@ -132,7 +136,7 @@ tryHandlingLoadableVarMovePattern(MarkUnresolvedMoveAddrInst *markMoveAddr,
         }
 
         if (auto *dvi = dyn_cast<DestroyAddrInst>(&iter)) {
-          if (aa->isNoAlias(dvi->getOperand(), originalASI)) {
+          if (aa->isNoAlias(dvi->getOperand(), operand)) {
             // We are going to be extending the lifetime of our
             // underlying value, not shrinking it so we can ignore
             // destroy_addr on other non-aliasing values.
@@ -157,7 +161,7 @@ tryHandlingLoadableVarMovePattern(MarkUnresolvedMoveAddrInst *markMoveAddr,
   // Ok, we know our original lexical alloc_stack is not written to in between
   // the load/store. Move the mark_move_addr onto the lexical alloc_stack.
   LLVM_DEBUG(llvm::dbgs() << "        Doing loadable var!\n");
-  markMoveAddr->setSrc(originalASI);
+  markMoveAddr->setSrc(operand);
   return true;
 }
 
@@ -239,13 +243,13 @@ static bool tryConvertSimpleMoveFromAllocStackTemporary(
   }
 
   // If we have a store [init], see if our src is a load [copy] from an
-  // alloc_stack that is lexical var. In this case, we want to move our
-  // mark_unresolved_move_addr onto that lexical var. This pattern occurs due to
-  // SILGen always loading loadable values from memory when retrieving an
-  // RValue. Calling _move then since _move is generic forces the value to be
-  // re-materialized into an alloc_stack. In this example remembering that
-  // mark_unresolved_move_addr is a copy_addr [init], we try to move the MUMA
-  // onto the original lexical alloc_stack.
+  // alloc_stack that is lexical var or an inout argument. In this case, we want
+  // to move our mark_unresolved_move_addr onto that lexical var. This pattern
+  // occurs due to SILGen always loading loadable values from memory when
+  // retrieving an RValue. Calling _move then since _move is generic forces the
+  // value to be re-materialized into an alloc_stack. In this example
+  // remembering that mark_unresolved_move_addr is a copy_addr [init], we try to
+  // move the MUMA onto the original lexical alloc_stack.
   if (tryHandlingLoadableVarMovePattern(markMoveAddr, si, aa))
     return true;
 
diff --git a/lib/SILOptimizer/Mandatory/MoveKillsCopyableAddressesChecker.cpp b/lib/SILOptimizer/Mandatory/MoveKillsCopyableAddressesChecker.cpp
@@ -885,6 +885,20 @@ bool MoveKillsCopyableAddressesObjectChecker::check() {
     if (!visitAccessPathUses(visitor, accessPath, fn))
       continue;
 
+    // See if our base address is an inout. If we found any moves, add as a
+    // liveness use all function terminators.
+    if (auto *fArg = dyn_cast<SILFunctionArgument>(address)) {
+      if (fArg->hasConvention(SILArgumentConvention::Indirect_Inout)) {
+        if (visitor.useState.markMoves.size()) {
+          SmallVector<SILBasicBlock *, 2> exitingBlocks;
+          fn->findExitingBlocks(exitingBlocks);
+          for (auto *block : exitingBlocks) {
+            visitor.useState.livenessUses.insert(block->getTerminator());
+          }
+        }
+      }
+    }
+
     // Now initialize our data structures.
     SWIFT_DEFER {
       useBlocks.clear();
@@ -987,7 +1001,7 @@ class MoveKillsCopyableAddressesCheckerPass : public SILFunctionTransform {
     for (auto *arg : fn->front().getSILFunctionArguments()) {
       if (arg->getType().isAddress() &&
           (arg->hasConvention(SILArgumentConvention::Indirect_In) ||
-           arg->hasConvention(SILArgumentConvention::Indirect_In)))
+           arg->hasConvention(SILArgumentConvention::Indirect_Inout)))
         checker.addressesToCheck.insert(arg);
     }
 
diff --git a/test/SILOptimizer/move_function_kills_copyable_addressonly_vars.swift b/test/SILOptimizer/move_function_kills_copyable_addressonly_vars.swift
@@ -13,6 +13,7 @@ public class Klass {}
 func consumingUse<T>(_ k: __owned T) {}
 var booleanValue: Bool { false }
 func nonConsumingUse<T>(_ k: T) {}
+func exchangeUse<T>(_ k: __owned T) -> T { k }
 
 ///////////
 // Tests //
@@ -93,3 +94,106 @@ public func performMoveOnVarMultiBlockError2<T>(_ p: T) {
     x = p
     nonConsumingUse(x)
 }
+
+public func performMoveOnInOut<T>(_ p: inout T) { // expected-error {{'p' used after being moved}}
+    let buf = _move(p) // expected-note {{move here}}
+    let _ = buf
+} // expected-note {{use here}}
+
+public func performMoveOnInOut2<T>(_ p: inout T, _ p2: T) {
+    let buf = _move(p)
+    let _ = buf
+    p = p2
+}
+
+struct S<T> {
+    var buffer: T?
+
+    mutating func appendNoError() {
+        let b = _move(self).buffer
+        let maybeNewB = exchangeUse(b)
+        self = .init(buffer: maybeNewB)
+    }
+
+    mutating func appendError() { // expected-error {{'self' used after being moved}}
+        let b = _move(self).buffer // expected-note {{move here}}
+        let _ = b
+    } // expected-note {{use here}}
+
+    mutating func appendThrowingNoError1(_ f: () throws -> ()) throws {
+        let b = _move(self).buffer
+        let maybeNewB = exchangeUse(b)
+        // We have to initialize self before we call try since otherwise we will
+        // not initialize self along the throws path.
+        self = .init(buffer: maybeNewB)
+        try f()
+    }
+
+    mutating func appendThrowingNoError2(_ f: () throws -> ()) {
+        do {
+            let b = _move(self).buffer
+            try f()
+            let maybeNewB = exchangeUse(b)
+            self = .init(buffer: maybeNewB)
+        } catch {
+            self = .init(buffer: nil)
+        }
+    }
+
+    // In this case, since we initialize self before the try point, we will have
+    // re-initialized self before hitting either the code after the try that is
+    // inline or the catch block.
+    mutating func appendThrowingNoError3(_ f: () throws -> ()) {
+        do {
+            let b = _move(self).buffer
+            let maybeNewB = exchangeUse(b)
+            self = .init(buffer: maybeNewB)
+            try f()
+        } catch {
+        }
+    }
+
+    mutating func appendThrowingError0(_ f: () throws -> ()) throws { // expected-error {{'self' used after being moved}}
+        let b = _move(self).buffer // expected-note {{move here}}
+        let maybeNewB = exchangeUse(b)
+        try f() // expected-note {{use here}}
+        self = .init(buffer: maybeNewB)
+    }
+
+
+    mutating func appendThrowingError1(_ f: () throws -> ()) throws { // expected-error {{'self' used after being moved}}
+        let b = _move(self).buffer // expected-note {{move here}}
+        let maybeNewB = exchangeUse(b)
+        let _ = maybeNewB
+        try f() // expected-note {{use here}}
+    }
+
+    mutating func appendThrowingError2(_ f: () throws -> ()) { // expected-error {{'self' used after being moved}}
+        do {
+            let b = _move(self).buffer // expected-note {{move here}}
+            let _ = b
+            try f()
+        } catch {
+            self = .init(buffer: nil)
+        }
+    } // expected-note {{use here}}
+
+    mutating func appendThrowingError3(_ f: () throws -> ()) { // expected-error {{'self' used after being moved}}
+        do {
+            let b = _move(self).buffer // expected-note {{move here}}
+            try f()
+            let maybeNewB = exchangeUse(b)
+            self = .init(buffer: maybeNewB)
+        } catch {
+        }
+    } // expected-note {{use here}}
+
+    mutating func appendThrowingError4(_ f: () throws -> ()) { // expected-error {{'self' used after being moved}}
+        do {
+            let b = _move(self).buffer // expected-note {{move here}}
+            let _ = b
+            try f()
+        } catch {
+        }
+    } // expected-note {{use here}}
+}
diff --git a/test/SILOptimizer/move_function_kills_copyable_loadable_vars.swift b/test/SILOptimizer/move_function_kills_copyable_loadable_vars.swift
@@ -14,6 +14,7 @@ func consumingUse(_ k: __owned Klass) {}
 var booleanValue: Bool { false }
 var booleanValue2: Bool { false }
 func nonConsumingUse(_ k: Klass) {}
+func exchangeUse(_ k: Klass) -> Klass { k }
 
 ///////////
 // Klassests //
@@ -165,3 +166,107 @@ public func performMoveOnLaterDefinedInit2(_ p: Klass) {
     nonConsumingUse(x)
     let _ = _move(x)
 }
+
+public func performMoveOnInOut(_ p: inout Klass) { // expected-error {{'p' used after being moved}}
+    let buf = _move(p) // expected-note {{move here}}
+    let _ = buf
+} // expected-note {{use here}}
+
+public func performMoveOnInOut2(_ p: inout Klass, _ p2: Klass) {
+    let buf = _move(p)
+    p = p2
+    let _ = buf
+}
+
+// Mutating self is an inout argument.
+struct S {
+    var buffer: Klass?
+
+    mutating func appendNoError() {
+        let b = _move(self).buffer!
+        let maybeNewB = exchangeUse(b)
+        self = .init(buffer: maybeNewB)
+    }
+
+    mutating func appendError() { // expected-error {{'self' used after being moved}}
+        let b = _move(self).buffer // expected-note {{move here}}
+        let _ = b
+    } // expected-note {{use here}}
+
+    mutating func appendThrowingNoError1(_ f: () throws -> ()) throws {
+        let b = _move(self).buffer!
+        let maybeNewB = exchangeUse(b)
+        // We have to initialize self before we call try since otherwise we will
+        // not initialize self along the throws path.
+        self = .init(buffer: maybeNewB)
+        try f()
+    }
+
+    mutating func appendThrowingNoError2(_ f: () throws -> ()) {
+        do {
+            let b = _move(self).buffer!
+            try f()
+            let maybeNewB = exchangeUse(b)
+            self = .init(buffer: maybeNewB)
+        } catch {
+            self = .init(buffer: nil)
+        }
+    }
+
+    // In this case, since we initialize self before the try point, we will have
+    // re-initialized self before hitting either the code after the try that is
+    // inline or the catch block.
+    mutating func appendThrowingNoError3(_ f: () throws -> ()) {
+        do {
+            let b = _move(self).buffer!
+            let maybeNewB = exchangeUse(b)
+            self = .init(buffer: maybeNewB)
+            try f()
+        } catch {
+        }
+    }
+
+    mutating func appendThrowingError0(_ f: () throws -> ()) throws { // expected-error {{'self' used after being moved}}
+        let b = _move(self).buffer! // expected-note {{move here}}
+        let maybeNewB = exchangeUse(b)
+        try f() // expected-note {{use here}}
+        self = .init(buffer: maybeNewB)
+    }
+
+
+    mutating func appendThrowingError1(_ f: () throws -> ()) throws { // expected-error {{'self' used after being moved}}
+        let b = _move(self).buffer! // expected-note {{move here}}
+        let maybeNewB = exchangeUse(b)
+        let _ = maybeNewB
+        try f() // expected-note {{use here}}
+    }
+
+    mutating func appendThrowingError2(_ f: () throws -> ()) { // expected-error {{'self' used after being moved}}
+        do {
+            let b = _move(self).buffer // expected-note {{move here}}
+            let _ = b
+            try f()
+        } catch {
+            self = .init(buffer: nil)
+        }
+    } // expected-note {{use here}}
+
+    mutating func appendThrowingError3(_ f: () throws -> ()) { // expected-error {{'self' used after being moved}}
+        do {
+            let b = _move(self).buffer! // expected-note {{move here}}
+            try f()
+            let maybeNewB = exchangeUse(b)
+            self = .init(buffer: maybeNewB)
+        } catch {
+        }
+    } // expected-note {{use here}}
+
+    mutating func appendThrowingError4(_ f: () throws -> ()) { // expected-error {{'self' used after being moved}}
+        do {
+            let b = _move(self).buffer // expected-note {{move here}}
+            let _ = b
+            try f()
+        } catch {
+        }
+    } // expected-note {{use here}}
+}
