Add SIL verification for borrow and mutate accessors and some other minor fixes

SwiftCompilerSources/Sources/Optimizer/Analysis/AliasAnalysis.swift

@@ -775,7 +775,7 @@ private struct FullApplyEffectsVisitor : EscapeVisitorWithResult {
 
   mutating func visitUse(operand: Operand, path: EscapePath) -> UseResult {
     let user = operand.instruction
-    if user is ReturnInst {
+    if user is ReturnInstruction {
       // Anything which is returned cannot escape to an instruction inside the function.
       return .ignore
     }
@@ -805,7 +805,7 @@ private struct PartialApplyEffectsVisitor : EscapeVisitorWithResult {
 
   mutating func visitUse(operand: Operand, path: EscapePath) -> UseResult {
     let user = operand.instruction
-    if user is ReturnInst {
+    if user is ReturnInstruction {
       // Anything which is returned cannot escape to an instruction inside the function.
       return .ignore
     }
@@ -848,7 +848,7 @@ private struct EscapesToInstructionVisitor : EscapeVisitor {
     if user == target {
       return .abort
     }
-    if user is ReturnInst {
+    if user is ReturnInstruction {
       // Anything which is returned cannot escape to an instruction inside the function.
       return .ignore
     }

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/ComputeEscapeEffects.swift

@@ -81,7 +81,7 @@ let computeEscapeEffects = FunctionPass(name: "compute-escape-effects") {
 private
 func addArgEffects(_ arg: FunctionArgument, argPath ap: SmallProjectionPath,
                    to newEffects: inout [EscapeEffects.ArgumentEffect],
-                   _ returnInst: ReturnInst?, _ context: FunctionPassContext) -> Bool {
+                   _ returnInst: ReturnInstruction?, _ context: FunctionPassContext) -> Bool {
   // Correct the path if the argument is not a class reference itself, but a value type
   // containing one or more references.
   let argPath = arg.type.isClass ? ap : ap.push(.anyValueFields)
@@ -157,7 +157,7 @@ private struct ArgEffectsVisitor : EscapeVisitorWithResult {
   var result = EscapeDestination.notSet
 
   mutating func visitUse(operand: Operand, path: EscapePath) -> UseResult {
-    if operand.instruction is ReturnInst {
+    if operand.instruction is ReturnInstruction {
       // The argument escapes to the function return
       if path.followStores {
         // The escaping path must not introduce a followStores.
@@ -209,13 +209,13 @@ private struct IsExclusiveReturnEscapeVisitor : EscapeVisitorWithResult {
   let returnPath: SmallProjectionPath
   var result = false
 
-  func isExclusiveEscape(returnInst: ReturnInst, _ context: FunctionPassContext) -> Bool {
+  func isExclusiveEscape(returnInst: ReturnInstruction, _ context: FunctionPassContext) -> Bool {
     return returnInst.returnedValue.at(returnPath).visit(using: self, context) ?? false
   }
 
   mutating func visitUse(operand: Operand, path: EscapePath) -> UseResult {
     switch operand.instruction {
-    case is ReturnInst:
+    case is ReturnInstruction:
       if path.followStores { return .abort }
       if path.projectionPath.matches(pattern: returnPath) {
         return .ignore

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/ComputeSideEffects.swift

@@ -254,7 +254,7 @@ private struct CollectedEffects {
       // effect, it would not give any significant benefit in any of our current optimizations.
       addEffects(.destroy, to: inst.operands[0].value, fromInitialPath: SmallProjectionPath(.anyValueFields))
 
-    case is ReturnInst:
+    case is ReturnInstruction:
       if inst.parentFunction.convention.hasAddressResult {
         addEffects(.read, to: inst.operands[0].value)
       }
@@ -532,7 +532,7 @@ private struct ArgumentEscapingWalker : ValueDefUseWalker, AddressDefUseWalker {
     case is CopyValueInst, is RetainValueInst, is StrongRetainInst,
          is DestroyValueInst, is ReleaseValueInst, is StrongReleaseInst,
          is DebugValueInst, is UnconditionalCheckedCastInst,
-         is ReturnInst:
+         is ReturnInstruction:
       return .continueWalk
 
     case let apply as ApplySite:
@@ -620,7 +620,7 @@ private extension PartialApplyInst {
           // Any escape to apply - regardless if it's an argument or the callee operand - might cause
           // the closure to be called.
           return .abort
-        case is ReturnInst:
+        case is ReturnInstruction:
           return .ignore
         default:
           return .continueWalk

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LifetimeDependenceScopeFixup.swift

@@ -894,7 +894,7 @@ extension ExtendableScope {
         assert(end.parentBlock.singleSuccessor!.terminator is ReturnInst,
                "a phi only ends a use range if it is a returned value")
         fallthrough
-      case is ReturnInst:
+      case is ReturnInstruction:
         // End this inner scope just before the return. The mark_dependence base operand will be redirected to a
         // function argument.
         let builder = Builder(before: end, location: location, context)

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/PackSpecialization.swift

@@ -682,7 +682,7 @@ private struct PackExplodedFunction {
       if resultMap.contains(where: {
         $0.expandedElements.contains(where: { !$0.isSILIndirect })
       }) {
-        if let originalReturnStatement = specialized.returnInstruction {
+        if let originalReturnStatement = specialized.returnInstruction as? ReturnInst {
           self.createExplodedReturn(
             replacing: originalReturnStatement, argumentMap: argumentMap, specContext)
         }

SwiftCompilerSources/Sources/Optimizer/TestPasses/EscapeInfoDumper.swift

@@ -26,7 +26,7 @@ let escapeInfoDumper = FunctionPass(name: "dump-escape-info") {
     var result: Set<String> =  Set()
 
     mutating func visitUse(operand: Operand, path: EscapePath) -> UseResult {
-      if operand.instruction is ReturnInst {
+      if operand.instruction is ReturnInstruction {
         result.insert("return[\(path.projectionPath)]")
         return .ignore
       }
@@ -94,7 +94,7 @@ let addressEscapeInfoDumper = FunctionPass(name: "dump-addr-escape-info") {
       if user == apply {
         return .abort
       }
-      if user is ReturnInst {
+      if user is ReturnInstruction {
         // Anything which is returned cannot escape to an instruction inside the function.
         return .ignore
       }

SwiftCompilerSources/Sources/Optimizer/Utilities/EscapeUtils.swift

@@ -387,7 +387,7 @@ fileprivate struct EscapeWalker<V: EscapeVisitor> : ValueDefUseWalker,
       if handleDestroy(of: operand.value, path: path) == .abortWalk {
         return .abortWalk
       }
-    case is ReturnInst:
+    case is ReturnInstruction:
       return isEscaping
     case is ApplyInst, is TryApplyInst, is BeginApplyInst:
       return walkDownCallee(argOp: operand, apply: instruction as! FullApplySite, path: path)
@@ -534,7 +534,7 @@ fileprivate struct EscapeWalker<V: EscapeVisitor> : ValueDefUseWalker,
       if handleDestroy(of: operand.value, path: path) == .abortWalk {
         return .abortWalk
       }
-    case is ReturnInst:
+    case is ReturnInstruction:
       return isEscaping
     case is ApplyInst, is TryApplyInst, is BeginApplyInst:
       return walkDownCallee(argOp: operand, apply: instruction as! FullApplySite, path: path)

SwiftCompilerSources/Sources/Optimizer/Utilities/LifetimeDependenceUtils.swift

@@ -730,7 +730,7 @@ extension LifetimeDependenceDefUseWalker {
     if let apply = operand.instruction as? FullApplySite {
       return visitAppliedUse(of: operand, by: apply)
     }
-    if operand.instruction is ReturnInst, !operand.value.isEscapable {
+    if operand.instruction is ReturnInstruction, !operand.value.isEscapable {
       return returnedDependence(result: operand)
     }
     if operand.instruction is YieldInst, !operand.value.isEscapable {

SwiftCompilerSources/Sources/Optimizer/Utilities/OptUtils.swift

@@ -880,7 +880,7 @@ private struct EscapesToValueVisitor : EscapeVisitor {
     if operand.value == target.value && path.projectionPath.mayOverlap(with: target.path) {
       return .abort
     }
-    if operand.instruction is ReturnInst {
+    if operand.instruction is ReturnInstruction {
       // Anything which is returned cannot escape to an instruction inside the function.
       return .ignore
     }

SwiftCompilerSources/Sources/SIL/Argument.swift

@@ -212,7 +212,7 @@ public struct Phi {
 extension Phi {
   /// Return true of this phi is directly returned with no side effects between the phi and the return.
   public var isReturnValue: Bool {
-    if let singleUse = value.uses.singleUse, let ret = singleUse.instruction as? ReturnInst,
+    if let singleUse = value.uses.singleUse, let ret = singleUse.instruction as? ReturnInstruction,
        ret.parentBlock == successor {
       for inst in successor.instructions {
         if inst.mayHaveSideEffects {

SwiftCompilerSources/Sources/SIL/Function.swift

@@ -121,9 +121,9 @@ final public class Function : CustomStringConvertible, HasShortDescription, Hash
     blocks.reversed().lazy.flatMap { $0.instructions.reversed() }
   }
 
-  public var returnInstruction: ReturnInst? {
+  public var returnInstruction: ReturnInstruction? {
     for block in blocks.reversed() {
-      if let retInst = block.terminator as? ReturnInst { return retInst }
+      if let retInst = block.terminator as? ReturnInstruction { return retInst }
     }
     return nil
   }

SwiftCompilerSources/Sources/SIL/Utilities/WalkUtils.swift

@@ -838,6 +838,9 @@ extension AddressUseDefWalker {
       } else {
         return walkUp(address: ia.base, path: path.push(.anyIndexedElement, index: 0))
       }
+    case let apply as ApplyInst:
+      let selfArgument = apply.arguments.last!
+      return walkUp(address: selfArgument, path: path.push(.anyValueFields, index: 0))
     case is BeginAccessInst,
          is MarkDependenceInst,
          is MarkUninitializedInst,

include/swift/SIL/SILFunctionConventions.h

@@ -338,14 +338,25 @@ class SILFunctionConventions {
   }
 
   bool hasAddressResult() const {
+    return hasGuaranteedAddressResult() || hasInoutResult();
+  }
+
+  bool hasGuaranteedAddressResult() const {
     if (funcTy->getNumResults() != 1) {
       return false;
     }
+    if (!silConv.loweredAddresses) {
+      return false;
+    }
     auto resultConvention = funcTy->getResults()[0].getConvention();
-    if (silConv.loweredAddresses) {
-      return resultConvention == ResultConvention::GuaranteedAddress ||
-             resultConvention == ResultConvention::Inout;
+    return resultConvention == ResultConvention::GuaranteedAddress;
+  }
+
+  bool hasInoutResult() const {
+    if (funcTy->getNumResults() != 1) {
+      return false;
     }
+    auto resultConvention = funcTy->getResults()[0].getConvention();
     return resultConvention == ResultConvention::Inout;
   }
 

include/swift/SIL/SILInstruction.h

@@ -3155,9 +3155,15 @@ class ApplyInst final
     return getSubstCalleeConv().hasGuaranteedResult();
   }
 
+  bool hasGuaranteedAddressResult() const {
+    return getSubstCalleeConv().hasGuaranteedAddressResult();
+  }
+
   bool hasAddressResult() const {
     return getSubstCalleeConv().hasAddressResult();
   }
+
+  bool hasInoutResult() const { return getSubstCalleeConv().hasInoutResult(); }
 };
 
 /// PartialApplyInst - Represents the creation of a closure object by partial

lib/SIL/Utils/MemoryLocations.cpp

@@ -66,10 +66,15 @@ MemoryLocations::Location::Location(SILValue val, unsigned index, int parentIdx)
       representativeValue(val),
       parentIdx(parentIdx) {
   assert(((parentIdx >= 0) ==
-    (isa<StructElementAddrInst>(val) || isa<TupleElementAddrInst>(val) ||
-     isa<InitEnumDataAddrInst>(val) || isa<UncheckedTakeEnumDataAddrInst>(val) ||
-     isa<InitExistentialAddrInst>(val) || isa<OpenExistentialAddrInst>(val)))
-    && "sub-locations can only be introduced with struct/tuple/enum projections");
+          (isa<StructElementAddrInst>(val) || isa<TupleElementAddrInst>(val) ||
+           isa<InitEnumDataAddrInst>(val) ||
+           isa<UncheckedTakeEnumDataAddrInst>(val) ||
+           isa<InitExistentialAddrInst>(val) ||
+           isa<OpenExistentialAddrInst>(val) || isa<ApplyInst>(val) ||
+           isa<StoreBorrowInst>(val))) &&
+         "sub-locations can only be introduced with "
+         "struct/tuple/enum/store_borrow/borrow accessor "
+         "projections");
   setBitAndResize(subLocations, index);
   setBitAndResize(selfAndParents, index);
 }
@@ -349,6 +354,21 @@ bool MemoryLocations::analyzeLocationUsesRecursively(SILValue V, unsigned locIdx
         if (cast<DebugValueInst>(user)->hasAddrVal())
           break;
         return false;
+      case SILInstructionKind::ApplyInst: {
+        auto *apply = cast<ApplyInst>(user);
+        if (apply->hasAddressResult()) {
+          if (!analyzeAddrProjection(apply, locIdx, 0, collectedVals,
+                                     subLocationMap))
+            return false;
+        }
+        break;
+      }
+      case SILInstructionKind::StoreBorrowInst: {
+        if (!analyzeAddrProjection(cast<StoreBorrowInst>(user), locIdx, 0,
+                                   collectedVals, subLocationMap))
+          return false;
+        break;
+      }
       case SILInstructionKind::InjectEnumAddrInst:
       case SILInstructionKind::SelectEnumAddrInst:
       case SILInstructionKind::ExistentialMetatypeInst:
@@ -357,20 +377,19 @@ bool MemoryLocations::analyzeLocationUsesRecursively(SILValue V, unsigned locIdx
       case SILInstructionKind::FixLifetimeInst:
       case SILInstructionKind::LoadInst:
       case SILInstructionKind::StoreInst:
-      case SILInstructionKind::StoreBorrowInst:
       case SILInstructionKind::EndAccessInst:
       case SILInstructionKind::DestroyAddrInst:
       case SILInstructionKind::CheckedCastAddrBranchInst:
       case SILInstructionKind::UncheckedRefCastAddrInst:
       case SILInstructionKind::UnconditionalCheckedCastAddrInst:
-      case SILInstructionKind::ApplyInst:
       case SILInstructionKind::TryApplyInst:
       case SILInstructionKind::BeginApplyInst:
       case SILInstructionKind::CopyAddrInst:
       case SILInstructionKind::YieldInst:
       case SILInstructionKind::DeallocStackInst:
       case SILInstructionKind::SwitchEnumAddrInst:
       case SILInstructionKind::WitnessMethodInst:
+      case SILInstructionKind::EndBorrowInst:
         break;
       case SILInstructionKind::MarkUnresolvedMoveAddrInst:
         // We do not want the memory lifetime verifier to verify move_addr inst

lib/SIL/Verifier/MemoryLifetimeVerifier.cpp

@@ -335,7 +335,7 @@ bool MemoryLifetimeVerifier::applyMayRead(Operand *argOp, SILValue addr) {
 
 void MemoryLifetimeVerifier::requireNoStoreBorrowLocation(
     SILValue addr, SILInstruction *where) {
-  if (isa<StoreBorrowInst>(addr)) {
+  if (isStoreBorrowLocation(addr)) {
     reportError("store-borrow location cannot be written",
                 locations.getLocationIdx(addr), where);
   }

lib/SIL/Verifier/SILVerifier.cpp

@@ -5359,6 +5359,17 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
                instResultType.dump(););
     requireSameType(functionResultType, instResultType,
                     "return value type does not match return type of function");
+
+    // If the result type is an address, ensure it's base address is from a
+    // function argument.
+    if (F.getModule().getStage() >= SILStage::Canonical &&
+        functionResultType.isAddress()) {
+      auto base = getAccessBase(RI->getOperand());
+      require(!base->getType().isAddress() || isa<SILFunctionArgument>(base) ||
+                  isa<ApplyInst>(base) &&
+                      cast<ApplyInst>(base)->hasAddressResult(),
+              "unidentified address return");
+    }
   }
 
   void checkThrowInst(ThrowInst *TI) {

lib/SILGen/SILGenApply.cpp

@@ -5483,15 +5483,23 @@ ManagedValue SILGenFunction::applyBorrowMutateAccessor(
   if (fn.getFunction()->getConventions().hasGuaranteedResult()) {
     auto selfArg = args.back().getValue();
     if (isa<LoadBorrowInst>(selfArg)) {
+      // unchecked_ownership is used to silence the ownership verifier for
+      // returning a value produced within a load_borrow scope. SILGenCleanup
+      // eliminates it and introduces return_borrow appropriately.
       rawResult = B.createUncheckedOwnership(loc, rawResult);
     }
   }
 
   if (rawResult->getType().isMoveOnly()) {
     if (rawResult->getType().isAddress()) {
+      SILFunctionConventions substFnConv(substFnType, SGM.M);
       auto result = B.createMarkUnresolvedNonCopyableValueInst(
           loc, rawResult,
-          MarkUnresolvedNonCopyableValueInst::CheckKind::NoConsumeOrAssign);
+          substFnConv.hasInoutResult()
+              ? MarkUnresolvedNonCopyableValueInst::CheckKind::
+                    AssignableButNotConsumable
+              : MarkUnresolvedNonCopyableValueInst::CheckKind::
+                    NoConsumeOrAssign);
       return ManagedValue::forRValueWithoutOwnership(result);
     }
     auto result = emitManagedCopy(loc, rawResult);

lib/SILGen/SILGenStmt.cpp

@@ -720,6 +720,9 @@ bool SILGenFunction::emitBorrowOrMutateAccessorResult(
     assert(F.getConventions().hasGuaranteedResult());
     auto regularLoc = RegularLocation::getAutoGeneratedLocation();
     auto load = B.createLoadBorrow(regularLoc, guaranteedAddress).getValue();
+    // unchecked_ownership is used to silence the ownership verifier for
+    // returning a value produced within a load_borrow scope. SILGenCleanup
+    // eliminates it and introduces return_borrow appropriately.
     return B.createUncheckedOwnership(regularLoc, load);
   };
 

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

@@ -472,6 +472,22 @@ static bool visitScopeEndsRequiringInit(
     return true;
   }
 
+  // Check for mutate accessor.
+  if (auto ai =
+          dyn_cast_or_null<ApplyInst>(operand->getDefiningInstruction())) {
+    if (ai->hasInoutResult()) {
+      auto *access =
+          dyn_cast<BeginAccessInst>(getAccessScope(ai->getSelfArgument()));
+      if (access) {
+        assert(access->getAccessKind() == SILAccessKind::Modify);
+        for (auto *inst : access->getEndAccesses()) {
+          visit(inst, ScopeRequiringFinalInit::ModifyMemoryAccess);
+        }
+        return true;
+      }
+      return false;
+    }
+  }
   return false;
 }