Merge pull request #39736 from atrick/cse-clonecheck

CSE/OSSA avoid endless cloning and reoptimization

Author: atrick

include/swift/SILOptimizer/Utils/OwnershipOptUtils.h

@@ -71,9 +71,10 @@ struct OwnershipFixupContext {
     /// and use this to seed that new lifetime.
     SmallVector<Operand *, 8> allAddressUsesFromOldValue;
 
-    /// This is the interior pointer operand that the new value we want to RAUW
-    /// is transitively derived from and enables us to know the underlying
-    /// borrowed base value that we need to lifetime extend.
+    /// This is the interior pointer (e.g. ref_element_addr)
+    /// that the new value we want to RAUW is transitively derived from and
+    /// enables us to know the underlying borrowed base value that we need to
+    /// lifetime extend.
     ///
     /// This is only initialized when the interior pointer has uses that must be
     /// replaced.
@@ -149,9 +150,15 @@ class OwnershipRAUWHelper {
   operator bool() const { return isValid(); }
   bool isValid() const { return bool(ctx) && bool(oldValue) && bool(newValue); }
 
-  /// Perform the actual RAUW. We require that \p newValue and \p oldValue have
-  /// the same type at this point (in contrast to when calling
-  /// OwnershipRAUWFixupHelper::get()).
+  /// True if replacement requires copying the original instruction's source
+  /// operand, creating a new borrow scope for that copy, then cloning the
+  /// original.
+  bool requiresCopyBorrowAndClone() const {
+    return ctx->extraAddressFixupInfo.base;
+  }
+
+  /// Perform OSSA fixup on newValue and return a fixed-up value based that can
+  /// be used to replace all uses of oldValue.
   ///
   /// This is so that we can avoid creating "forwarding" transformation
   /// instructions before we know if we can perform the RAUW. Any such

lib/SILOptimizer/Transforms/CSE.cpp

@@ -1033,7 +1033,10 @@ bool CSE::processNode(DominanceInfoNode *Node) {
         OwnershipRAUWHelper helper(RAUWFixupContext,
                                    cast<SingleValueInstruction>(Inst),
                                    cast<SingleValueInstruction>(AvailInst));
-        if (!helper.isValid())
+        // If RAUW requires cloning the original, then there's no point. If it
+        // also requires introducing a copy and new borrow scope, then it's a
+        // very bad idea.
+        if (!helper.isValid() || helper.requiresCopyBorrowAndClone())
           continue;
         // Replace SingleValueInstruction using OSSA RAUW here
         nextI = helper.perform();

lib/SILOptimizer/Utils/OwnershipOptUtils.cpp

@@ -1041,10 +1041,9 @@ OwnershipRAUWHelper::replaceAddressUses(SingleValueInstruction *oldValue,
                                         SILValue newValue) {
   assert(oldValue->getType().isAddress() && newValue->getType().isAddress());
 
-  // If we are replacing addresses, see if we need to handle interior pointer
-  // fixups. If we don't have any extra info, then we know that we can just RAUW
-  // without any further work.
-  if (!ctx->extraAddressFixupInfo.base)
+  // If newValue was not generated by an interior pointer, then it cannot
+  // be within a borrow scope, so direct replacement works.
+  if (!requiresCopyBorrowAndClone())
     return replaceAllUsesAndErase(oldValue, newValue, ctx->callbacks);
 
   // We are RAUWing two addresses and we found that: