[move-only] Eliminate some temporary copies inserted by SILGen when accessing fields of lets.

In the next commit, I am modifying the move only checker to ensure that we
always error when partially invalidating. While doing this I discovered that
these copies were getting in the way of emitting good diagnostics in that case.

Author: gottesmm

include/swift/SILOptimizer/PassManager/Passes.def

@@ -448,6 +448,8 @@ PASS(MoveOnlyAddressChecker, "sil-move-only-address-checker",
      "Utility pass that enforces move only invariants on raw SIL for addresses for testing purposes")
 PASS(MoveOnlyChecker, "sil-move-only-checker",
      "Pass that enforces move only invariants on raw SIL for addresses and objects")
+PASS(MoveOnlyTempAllocationFromLetTester, "sil-move-only-temp-allocation-from-let-tester",
+     "Pass that allows us to run separately SIL test cases for the eliminateTemporaryAllocationsFromLet utility")
 PASS(ConsumeOperatorCopyableValuesChecker, "sil-consume-operator-copyable-values-checker",
      "Pass that performs checking of the consume operator for copyable values")
 PASS(TrivialMoveOnlyTypeEliminator, "sil-trivial-move-only-type-eliminator",

lib/SILOptimizer/Mandatory/CMakeLists.txt

@@ -31,6 +31,7 @@ target_sources(swiftSILOptimizer PRIVATE
   MoveOnlyDiagnostics.cpp
   MoveOnlyObjectCheckerTester.cpp
   MoveOnlyObjectCheckerUtils.cpp
+  MoveOnlyTempAllocationFromLetTester.cpp
   MoveOnlyTypeUtils.cpp
   MoveOnlyUtils.cpp
   MovedAsyncVarDebugInfoPropagator.cpp

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

@@ -287,108 +287,9 @@ llvm::cl::opt<bool> DisableMoveOnlyAddressCheckerLifetimeExtension(
                    "move-only values."));
 
 //===----------------------------------------------------------------------===//
-//                           MARK: Memory Utilities
+//                              MARK: Utilities
 //===----------------------------------------------------------------------===//
 
-static bool memInstMustInitialize(Operand *memOper) {
-  SILValue address = memOper->get();
-
-  SILInstruction *memInst = memOper->getUser();
-
-  switch (memInst->getKind()) {
-  default:
-    return false;
-
-  case SILInstructionKind::CopyAddrInst: {
-    auto *CAI = cast<CopyAddrInst>(memInst);
-    return CAI->getDest() == address && CAI->isInitializationOfDest();
-  }
-  case SILInstructionKind::ExplicitCopyAddrInst: {
-    auto *CAI = cast<ExplicitCopyAddrInst>(memInst);
-    return CAI->getDest() == address && CAI->isInitializationOfDest();
-  }
-  case SILInstructionKind::MarkUnresolvedMoveAddrInst: {
-    return cast<MarkUnresolvedMoveAddrInst>(memInst)->getDest() == address;
-  }
-  case SILInstructionKind::InitExistentialAddrInst:
-  case SILInstructionKind::InitEnumDataAddrInst:
-  case SILInstructionKind::InjectEnumAddrInst:
-    return true;
-
-  case SILInstructionKind::BeginApplyInst:
-  case SILInstructionKind::TryApplyInst:
-  case SILInstructionKind::ApplyInst: {
-    FullApplySite applySite(memInst);
-    return applySite.isIndirectResultOperand(*memOper);
-  }
-  case SILInstructionKind::StoreInst: {
-    auto qual = cast<StoreInst>(memInst)->getOwnershipQualifier();
-    return qual == StoreOwnershipQualifier::Init ||
-           qual == StoreOwnershipQualifier::Trivial;
-  }
-
-#define NEVER_OR_SOMETIMES_LOADABLE_CHECKED_REF_STORAGE(Name, ...)             \
-  case SILInstructionKind::Store##Name##Inst:                                  \
-    return cast<Store##Name##Inst>(memInst)->isInitializationOfDest();
-#include "swift/AST/ReferenceStorage.def"
-  }
-}
-
-static bool memInstMustReinitialize(Operand *memOper) {
-  SILValue address = memOper->get();
-
-  SILInstruction *memInst = memOper->getUser();
-
-  switch (memInst->getKind()) {
-  default:
-    return false;
-
-  case SILInstructionKind::CopyAddrInst: {
-    auto *CAI = cast<CopyAddrInst>(memInst);
-    return CAI->getDest() == address && !CAI->isInitializationOfDest();
-  }
-  case SILInstructionKind::ExplicitCopyAddrInst: {
-    auto *CAI = cast<ExplicitCopyAddrInst>(memInst);
-    return CAI->getDest() == address && !CAI->isInitializationOfDest();
-  }
-  case SILInstructionKind::YieldInst: {
-    auto *yield = cast<YieldInst>(memInst);
-    return yield->getYieldInfoForOperand(*memOper).isIndirectInOut();
-  }
-  case SILInstructionKind::BeginApplyInst:
-  case SILInstructionKind::TryApplyInst:
-  case SILInstructionKind::ApplyInst: {
-    FullApplySite applySite(memInst);
-    return applySite.getArgumentOperandConvention(*memOper).isInoutConvention();
-  }
-  case SILInstructionKind::StoreInst:
-    return cast<StoreInst>(memInst)->getOwnershipQualifier() ==
-           StoreOwnershipQualifier::Assign;
-
-#define NEVER_OR_SOMETIMES_LOADABLE_CHECKED_REF_STORAGE(Name, ...)             \
-  case SILInstructionKind::Store##Name##Inst:                                  \
-    return !cast<Store##Name##Inst>(memInst)->isInitializationOfDest();
-#include "swift/AST/ReferenceStorage.def"
-  }
-}
-
-/// Is this a reinit instruction that we know how to convert into its init form.
-static bool isReinitToInitConvertibleInst(SILInstruction *memInst) {
-  switch (memInst->getKind()) {
-  default:
-    return false;
-
-  case SILInstructionKind::CopyAddrInst: {
-    auto *cai = cast<CopyAddrInst>(memInst);
-    return !cai->isInitializationOfDest();
-  }
-  case SILInstructionKind::StoreInst: {
-    auto *si = cast<StoreInst>(memInst);
-    return si->getOwnershipQualifier() == StoreOwnershipQualifier::Assign;
-  }
-  }
-}
-
 static void insertDebugValueBefore(SILInstruction *insertPt,
                                    DebugVarCarryingInst debugVar,
                                    llvm::function_ref<SILValue ()> operand) {
@@ -432,47 +333,20 @@ static void convertMemoryReinitToInitForm(SILInstruction *memInst,
                        [&]{ return debugVar.getOperandForDebugValueClone(); });
 }
 
-static bool memInstMustConsume(Operand *memOper) {
-  SILValue address = memOper->get();
-
-  SILInstruction *memInst = memOper->getUser();
-
-  // FIXME: drop_deinit must be handled here!
+/// Is this a reinit instruction that we know how to convert into its init form.
+static bool isReinitToInitConvertibleInst(SILInstruction *memInst) {
   switch (memInst->getKind()) {
   default:
     return false;
 
   case SILInstructionKind::CopyAddrInst: {
-    auto *CAI = cast<CopyAddrInst>(memInst);
-    return (CAI->getSrc() == address && CAI->isTakeOfSrc()) ||
-           (CAI->getDest() == address && !CAI->isInitializationOfDest());
-  }
-  case SILInstructionKind::ExplicitCopyAddrInst: {
-    auto *CAI = cast<ExplicitCopyAddrInst>(memInst);
-    return (CAI->getSrc() == address && CAI->isTakeOfSrc()) ||
-           (CAI->getDest() == address && !CAI->isInitializationOfDest());
-  }
-  case SILInstructionKind::BeginApplyInst:
-  case SILInstructionKind::TryApplyInst:
-  case SILInstructionKind::ApplyInst: {
-    FullApplySite applySite(memInst);
-    return applySite.getArgumentOperandConvention(*memOper).isOwnedConvention();
-  }
-  case SILInstructionKind::PartialApplyInst: {
-    // If we are on the stack or have an inout convention, we do not
-    // consume. Otherwise, we do.
-    auto *pai = cast<PartialApplyInst>(memInst);
-    if (pai->isOnStack())
-      return false;
-    ApplySite applySite(pai);
-    auto convention = applySite.getArgumentConvention(*memOper);
-    return !convention.isInoutConvention();
+    auto *cai = cast<CopyAddrInst>(memInst);
+    return !cai->isInitializationOfDest();
+  }
+  case SILInstructionKind::StoreInst: {
+    auto *si = cast<StoreInst>(memInst);
+    return si->getOwnershipQualifier() == StoreOwnershipQualifier::Assign;
   }
-  case SILInstructionKind::DestroyAddrInst:
-    return true;
-  case SILInstructionKind::LoadInst:
-    return cast<LoadInst>(memInst)->getOwnershipQualifier() ==
-           LoadOwnershipQualifier::Take;
   }
 }
 
@@ -1725,7 +1599,7 @@ bool GatherUsesVisitor::visitUse(Operand *op) {
   LLVM_DEBUG(llvm::dbgs() << "Visiting user: " << *user;);
 
   // First check if we have init/reinit. These are quick/simple.
-  if (::memInstMustInitialize(op)) {
+  if (noncopyable::memInstMustInitialize(op)) {
     LLVM_DEBUG(llvm::dbgs() << "Found init: " << *user);
 
     // TODO: What about copy_addr of itself. We really should just pre-process
@@ -1739,7 +1613,7 @@ bool GatherUsesVisitor::visitUse(Operand *op) {
     return true;
   }
 
-  if (::memInstMustReinitialize(op)) {
+  if (noncopyable::memInstMustReinitialize(op)) {
     LLVM_DEBUG(llvm::dbgs() << "Found reinit: " << *user);
     assert(!useState.reinitInsts.count(user));
     auto leafRange = TypeTreeLeafTypeRange::get(op->get(), getRootAddress());
@@ -2024,7 +1898,7 @@ bool GatherUsesVisitor::visitUse(Operand *op) {
 
   // Now that we have handled or loadTakeOrCopy, we need to now track our
   // additional pure takes.
-  if (::memInstMustConsume(op)) {
+  if (noncopyable::memInstMustConsume(op)) {
     // If we don't have a consumeable and assignable check kind, then we can't
     // consume. Emit an error.
     //
@@ -3188,6 +3062,24 @@ bool MoveOnlyAddressCheckerPImpl::performSingleCheck(
     state.process();
   }
 
+  // Then if we have a let allocation, see if we have any copy_addr on our
+  // markedAddress that form temporary allocation chains. This occurs when we
+  // emit SIL for code like:
+  //
+  // let x: AddressOnlyType = ...
+  // let _ = x.y.z
+  //
+  // SILGen will treat y as a separate rvalue from x and will create a temporary
+  // allocation. In contrast if we have a var, we treat x like an lvalue and
+  // just create GEPs appropriately.
+  if (eliminateTemporaryAllocationsFromLet(markedAddress)) {
+    LLVM_DEBUG(
+        llvm::dbgs()
+            << "Succeeded in eliminating temporary allocations! Fn after:\n";
+        markedAddress->getFunction()->dump());
+    changed = true;
+  }
+
   // 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.g.:
   // init, reinit, take, destroy, etc.).

lib/SILOptimizer/Mandatory/MoveOnlyTempAllocationFromLetTester.cpp

@@ -0,0 +1,78 @@
+//===--- MoveOnlyTempAllocationFromLetTester.cpp --------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2023 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file A simple tester for the utility function
+/// eliminateTemporaryAllocationsFromLet that allows us to write separate SIL
+/// test cases for the utility.
+///
+//===----------------------------------------------------------------------===//
+
+#define DEBUG_TYPE "sil-move-only-checker"
+
+#include "MoveOnlyAddressCheckerUtils.h"
+#include "MoveOnlyDiagnostics.h"
+#include "MoveOnlyUtils.h"
+
+#include "swift/SILOptimizer/PassManager/Passes.h"
+#include "swift/SILOptimizer/PassManager/Transforms.h"
+
+using namespace swift;
+using namespace swift::siloptimizer;
+
+namespace {
+
+struct MoveOnlyTempAllocationFromLetTester : SILFunctionTransform {
+  void run() override {
+    auto *fn = getFunction();
+
+    // Only run this pass if the move only language feature is enabled.
+    if (!fn->getASTContext().supportsMoveOnlyTypes())
+      return;
+
+    // Don't rerun diagnostics on deserialized functions.
+    if (getFunction()->wasDeserializedCanonical())
+      return;
+
+    assert(fn->getModule().getStage() == SILStage::Raw &&
+           "Should only run on Raw SIL");
+
+    LLVM_DEBUG(llvm::dbgs()
+               << "===> MoveOnlyTempAllocationFromLetTester. Visiting: "
+               << fn->getName() << '\n');
+
+    SmallSetVector<MarkMustCheckInst *, 32> moveIntroducersToProcess;
+    DiagnosticEmitter diagnosticEmitter(getFunction());
+
+    unsigned diagCount = diagnosticEmitter.getDiagnosticCount();
+    searchForCandidateAddressMarkMustChecks(
+        getFunction(), moveIntroducersToProcess, diagnosticEmitter);
+
+    // Return early if we emitted a diagnostic.
+    if (diagCount != diagnosticEmitter.getDiagnosticCount())
+      return;
+
+    bool madeChange = false;
+    while (!moveIntroducersToProcess.empty()) {
+      auto *next = moveIntroducersToProcess.pop_back_val();
+      madeChange |= eliminateTemporaryAllocationsFromLet(next);
+    }
+
+    if (madeChange)
+      invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
+  }
+};
+
+} // namespace
+
+SILTransform *swift::createMoveOnlyTempAllocationFromLetTester() {
+  return new MoveOnlyTempAllocationFromLetTester();
+}