Merge pull request #36540 from eeckstein/fix-temp-rvalue-opt

eeckstein · web-flow · commit 126850e57d96 · 2021-03-23T10:50:51.000+01:00
TempRValueOpt: extend access scopes
diff --git a/lib/SILOptimizer/Transforms/TempRValueElimination.cpp b/lib/SILOptimizer/Transforms/TempRValueElimination.cpp
@@ -89,6 +89,8 @@ class TempRValueOptPass : public SILFunctionTransform {
   bool
   checkTempObjectDestroy(AllocStackInst *tempObj, CopyAddrInst *copyInst);
 
+  bool extendAccessScopes(CopyAddrInst *copyInst, SILInstruction *lastUseInst);
+
   bool tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst);
   std::pair<SILBasicBlock::iterator, bool>
   tryOptimizeStoreIntoTemp(StoreInst *si);
@@ -167,12 +169,10 @@ collectLoads(Operand *addressUse, CopyAddrInst *originalCopy,
     //   %addr = begin_access [read]
     //      ... // there can be no writes to %addr here
     //   end_acess %addr   // <- This is where the use actually ends.
-    for (Operand *accessUse : beginAccess->getUses()) {
-      if (auto *endAccess = dyn_cast<EndAccessInst>(accessUse->getUser())) {
-        if (endAccess->getParent() != block)
-          return false;
-        loadInsts.insert(endAccess);
-      }
+    for (EndAccessInst *endAccess : beginAccess->getEndAccesses()) {
+      if (endAccess->getParent() != block)
+        return false;
+      loadInsts.insert(endAccess);
     }
     return true;
   }
@@ -347,6 +347,49 @@ SILInstruction *TempRValueOptPass::getLastUseWhileSourceIsNotModified(
   return nullptr;
 }
 
+/// Tries to move an end_access down to extend the access scope over all uses
+/// of the temporary. For example:
+///
+///   %a = begin_access %src
+///   copy_addr %a to [initialization] %temp : $*T
+///   end_access %a
+///   use %temp
+///
+/// We must not replace %temp with %a after the end_access. Instead we try to
+/// move the end_access after "use %temp".
+bool TempRValueOptPass::extendAccessScopes(
+    CopyAddrInst *copyInst, SILInstruction *lastUseInst) {
+
+  SILValue copySrc = copyInst->getSrc();
+  EndAccessInst *endAccessToMove = nullptr;
+  auto begin = std::next(copyInst->getIterator());
+  auto end = std::next(lastUseInst->getIterator());
+
+  for (SILInstruction &inst : make_range(begin, end)) {
+    if (auto *endAccess = dyn_cast<EndAccessInst>(&inst)) {
+      // Is this the end of an access scope of the copy-source?
+      if (!aa->isNoAlias(copySrc, endAccess->getSource())) {
+        // To keep things simple, we can just move a single end_access.
+        if (endAccessToMove)
+          return false;
+        endAccessToMove = endAccess;
+      }
+    } else if (endAccessToMove) {
+      // We cannot move an end_access over a begin_access. This would destroy
+      // the proper nesting of accesses.
+      if (isa<BeginAccessInst>(&inst))
+        return false;
+      // Don't extend a read-access scope over a (potential) write.
+      if (aa->mayWriteToMemory(&inst, endAccessToMove->getSource()))
+        return false;
+    }
+  }
+  if (endAccessToMove)
+    endAccessToMove->moveAfter(lastUseInst);
+
+  return true;
+}
+
 /// Return true if the \p tempObj, which is initialized by \p copyInst, is
 /// destroyed in an orthodox way.
 ///
@@ -419,13 +462,7 @@ bool TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {
 
   bool isOSSA = copyInst->getFunction()->hasOwnership();
   
-  // The copy's source address must not be a scoped instruction, like
-  // begin_borrow. When the temporary object is eliminated, it's uses are
-  // replaced with the copy's source. Therefore, the source address must be
-  // valid at least until the next instruction that may write to or destroy the
-  // source. End-of-scope markers, such as end_borrow, do not write to or
-  // destroy memory, so scoped addresses are not valid replacements.
-  SILValue copySrc = stripAccessMarkers(copyInst->getSrc());
+  SILValue copySrc = copyInst->getSrc();
   assert(tempObj != copySrc && "can't initialize temporary with itself");
 
   // If the source of the copyInst is taken, we must insert a compensating
@@ -487,6 +524,9 @@ bool TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {
   if (!isOSSA && !checkTempObjectDestroy(tempObj, copyInst))
     return false;
 
+  if (!extendAccessScopes(copyInst, lastLoadInst))
+    return false;
+
   LLVM_DEBUG(llvm::dbgs() << "  Success: replace temp" << *tempObj);
 
   if (needToInsertDestroy) {
@@ -529,7 +569,7 @@ bool TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {
       auto *li = cast<LoadInst>(user);
       if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Take)
         li->setOwnershipQualifier(LoadOwnershipQualifier::Copy);
-      use->set(copyInst->getSrc());
+      use->set(copySrc);
       break;
     }
 
@@ -702,7 +742,7 @@ void TempRValueOptPass::run() {
   bool changed = false;
 
   // Find all copy_addr instructions.
-  llvm::SmallVector<CopyAddrInst *, 8> deadCopies;
+  llvm::SmallSetVector<CopyAddrInst *, 8> deadCopies;
   for (auto &block : *getFunction()) {
     // Increment the instruction iterator only after calling
     // tryOptimizeCopyIntoTemp because the instruction after CopyInst might be
@@ -716,9 +756,9 @@ void TempRValueOptPass::run() {
         // calling tryOptimizeCopyIntoTemp or was created by an earlier
         // iteration, where another copy_addr copied the temporary back to the
         // source location.
-        if (stripAccessMarkers(copyInst->getSrc()) == copyInst->getDest()) {
+        if (copyInst->getSrc() == copyInst->getDest()) {
           changed = true;
-          deadCopies.push_back(copyInst);
+          deadCopies.insert(copyInst);
         }
         ++ii;
         continue;
@@ -735,21 +775,14 @@ void TempRValueOptPass::run() {
     }
   }
 
-  // Delete the copies and any unused address operands.
-  // The same copy may have been added multiple times.
-  sortUnique(deadCopies);
-  InstModCallbacks callbacks{
-#ifndef NDEBUG
-      // With asserts, we include this assert. Otherwise, we use the default
-      // impl for perf.
-      [](SILInstruction *instToKill) {
-        // SimplifyInstruction is not in the business of removing
-        // copy_addr. If it were, then we would need to update deadCopies.
-        assert(!isa<CopyAddrInst>(instToKill));
-        instToKill->eraseFromParent();
-      }
-#endif
-  };
+  InstModCallbacks callbacks(
+    [](SILInstruction *instToKill) {
+      // SimplifyInstruction is not in the business of removing
+      // copy_addr. If it were, then we would need to update deadCopies.
+      assert(!isa<CopyAddrInst>(instToKill));
+      instToKill->eraseFromParent();
+    }
+  );
 
   DeadEndBlocks deBlocks(getFunction());
   for (auto *deadCopy : deadCopies) {
diff --git a/test/SILOptimizer/sil_combine_protocol_conf.swift b/test/SILOptimizer/sil_combine_protocol_conf.swift
@@ -238,13 +238,15 @@ public class OtherClass {
 // CHECK: bb0([[ARG:%.*]] :
 // CHECK: debug_value
 // CHECK: [[R1:%.*]] = ref_element_addr [[ARG]] : $OtherClass, #OtherClass.arg1
-// CHECK: [[O1:%.*]] = open_existential_addr immutable_access [[R1]] : $*PropProtocol to $*@opened("{{.*}}") PropProtocol
+// CHECK: [[ACC1:%.*]] = begin_access [read] [static] [no_nested_conflict] [[R1]]
+// CHECK: [[O1:%.*]] = open_existential_addr immutable_access [[ACC1]] : $*PropProtocol to $*@opened("{{.*}}") PropProtocol
 // CHECK: [[U1:%.*]] = unchecked_addr_cast [[O1]] : $*@opened("{{.*}}") PropProtocol to $*PropClass 
 // CHECK: [[S1:%.*]] = struct_element_addr [[U1]] : $*PropClass, #PropClass.val
 // CHECK: [[S11:%.*]] = struct_element_addr [[S1]] : $*Int, #Int._value
 // CHECK: load [[S11]] 
 // CHECK: [[R2:%.*]] = ref_element_addr [[ARG]] : $OtherClass, #OtherClass.arg2
-// CHECK: [[O2:%.*]] = open_existential_addr immutable_access [[R2]] : $*GenericPropProtocol to $*@opened("{{.*}}") GenericPropProtocol
+// CHECK: [[ACC2:%.*]] = begin_access [read] [static] [no_nested_conflict] [[R2]]
+// CHECK: [[O2:%.*]] = open_existential_addr immutable_access [[ACC2]] : $*GenericPropProtocol to $*@opened("{{.*}}") GenericPropProtocol
 // CHECK: [[T1:%.*]] = alloc_stack $@opened
 // CHECK: copy_addr [[O2]] to [initialization] [[T1]]
 // CHECK: [[W2:%.*]] = witness_method $@opened("{{.*}}") GenericPropProtocol, #GenericPropProtocol.val!getter : <Self where Self : GenericPropProtocol> (Self) -> () -> Int, [[O2]] : $*@opened("{{.*}}") GenericPropProtocol : $@convention(witness_method: GenericPropProtocol) <τ_0_0 where τ_0_0 : GenericPropProtocol> (@in_guaranteed τ_0_0) -> Int
@@ -256,7 +258,8 @@ public class OtherClass {
 // CHECK: tuple_extract
 // CHECK: cond_fail
 // CHECK: [[R4:%.*]] = ref_element_addr [[ARG]] : $OtherClass, #OtherClass.arg3
-// CHECK: [[O4:%.*]] = open_existential_addr immutable_access [[R4]] : $*NestedPropProtocol to $*@opened("{{.*}}") NestedPropProtocol
+// CHECK: [[ACC4:%.*]] = begin_access [read] [static] [no_nested_conflict] [[R4]]
+// CHECK: [[O4:%.*]] = open_existential_addr immutable_access [[ACC4]] : $*NestedPropProtocol to $*@opened("{{.*}}") NestedPropProtocol
 // CHECK: [[U4:%.*]] = unchecked_addr_cast [[O4]] : $*@opened("{{.*}}") NestedPropProtocol to $*Outer.Inner
 // CHECK: [[S4:%.*]] = struct_element_addr [[U4]] : $*Outer.Inner, #Outer.Inner.val
 // CHECK: [[S41:%.*]] = struct_element_addr [[S4]] : $*Int, #Int._value
@@ -266,7 +269,8 @@ public class OtherClass {
 // CHECK: tuple_extract
 // CHECK: cond_fail
 // CHECK: [[R5:%.*]] = ref_element_addr [[ARG]] : $OtherClass, #OtherClass.arg4
-// CHECK: [[O5:%.*]] = open_existential_addr immutable_access [[R5]] : $*GenericNestedPropProtocol to $*@opened("{{.*}}") GenericNestedPropProtocol
+// CHECK: [[ACC5:%.*]] = begin_access [read] [static] [no_nested_conflict] [[R5]]
+// CHECK: [[O5:%.*]] = open_existential_addr immutable_access [[ACC5]] : $*GenericNestedPropProtocol to $*@opened("{{.*}}") GenericNestedPropProtocol
 // CHECK: [[T2:%.*]] = alloc_stack $@opened
 // CHECK: copy_addr [[O5]] to [initialization] [[T2]]
 // CHECK: [[W5:%.*]] = witness_method $@opened("{{.*}}") GenericNestedPropProtocol, #GenericNestedPropProtocol.val!getter : <Self where Self : GenericNestedPropProtocol> (Self) -> () -> Int, [[O5:%.*]] : $*@opened("{{.*}}") GenericNestedPropProtocol : $@convention(witness_method: GenericNestedPropProtocol) <τ_0_0 where τ_0_0 : GenericNestedPropProtocol> (@in_guaranteed τ_0_0) -> Int 
@@ -336,7 +340,8 @@ public class OtherKlass {
 // CHECK: debug_value
 // CHECK: integer_literal
 // CHECK: [[R1:%.*]] = ref_element_addr [[ARG]] : $OtherKlass, #OtherKlass.arg2
-// CHECK: [[O1:%.*]] = open_existential_addr immutable_access [[R1]] : $*AGenericProtocol to $*@opened("{{.*}}") AGenericProtocol
+// CHECK: [[ACC1:%.*]] = begin_access [read] [static] [no_nested_conflict] [[R1]]
+// CHECK: [[O1:%.*]] = open_existential_addr immutable_access [[ACC1]] : $*AGenericProtocol to $*@opened("{{.*}}") AGenericProtocol
 // CHECK: [[T1:%.*]] = alloc_stack $@opened
 // CHECK: copy_addr [[O1]] to [initialization] [[T1]]
 // CHECK: [[W1:%.*]] = witness_method $@opened("{{.*}}") AGenericProtocol, #AGenericProtocol.val!getter : <Self where Self : AGenericProtocol> (Self) -> () -> Int, [[O1]] : $*@opened("{{.*}}") AGenericProtocol : $@convention(witness_method: AGenericProtocol) <τ_0_0 where τ_0_0 : AGenericProtocol> (@in_guaranteed τ_0_0) -> Int
diff --git a/test/SILOptimizer/temp_rvalue_opt_ossa.sil b/test/SILOptimizer/temp_rvalue_opt_ossa.sil
@@ -42,6 +42,8 @@ bb0(%0 : $*Klass, %1 : $*Klass):
 sil @throwing_function : $@convention(thin) (@in_guaranteed Klass) -> ((), @error Error)
 sil @use_gsbase_builtinnativeobject : $@convention(thin) (@guaranteed GS<Builtin.NativeObject>) -> ()
 
+sil_global @globalString : $String
+
 ///////////
 // Tests //
 ///////////
@@ -830,16 +832,61 @@ bb3(%obj3 : @owned $GS<Builtin.NativeObject>):
   return %obj3 : $GS<Builtin.NativeObject>
 }
 
-// Test copy elimination with access markers on both the source and dest.
-//
-// CHECK-LABEL: sil [ossa] @takeWithAccess : $@convention(thin) (@in Klass) -> () {
+// CHECK-LABEL: sil [ossa] @extendAccessScopeOverLoad
+// CHECK:   [[GLOBAL:%.*]] = global_addr @globalString
+// CHECK:   [[ACCESS:%.*]] = begin_access [read] [dynamic] [[GLOBAL]]
+// CHECK:   [[LOAD:%.*]] = load [copy] [[ACCESS]]
+// CHECK:   end_access [[ACCESS]]
+// CHECK:   return [[LOAD]]
+// CHECK-LABEL: } // end sil function 'extendAccessScopeOverLoad'
+sil [ossa] @extendAccessScopeOverLoad : $@convention(thin) () -> @owned String {
+bb0:
+  %1 = global_addr @globalString : $*String
+  %3 = begin_access [read] [dynamic] %1 : $*String
+  %4 = alloc_stack $String
+  copy_addr %3 to [initialization] %4 : $*String
+  end_access %3 : $*String
+  %6 = load [copy] %4 : $*String
+  destroy_addr %4 : $*String
+  dealloc_stack %4 : $*String
+  return %6 : $String
+}
+
+sil [ossa] @loadString : $@convention(thin) (@in_guaranteed String) -> @owned String {
+bb0(%0 : $*String):
+  %1 = load [copy] %0 : $*String
+  return %1 : $String
+}
+
+// CHECK-LABEL: sil [ossa] @extendAccessScopeOverApply
+// CHECK:   [[GLOBAL:%.*]] = global_addr @globalString
+// CHECK:   [[ACCESS:%.*]] = begin_access [read] [dynamic] [[GLOBAL]]
+// CHECK:   [[RESULT:%.*]] = apply {{%[0-9]+}}([[ACCESS]])
+// CHECK:   end_access [[ACCESS]]
+// CHECK:   return [[RESULT]]
+// CHECK-LABEL: } // end sil function 'extendAccessScopeOverApply'
+sil [ossa] @extendAccessScopeOverApply : $@convention(thin) () -> @owned String {
+bb0:
+  %1 = global_addr @globalString : $*String
+  %3 = begin_access [read] [dynamic] %1 : $*String
+  %4 = alloc_stack $String
+  copy_addr %3 to [initialization] %4 : $*String
+  end_access %3 : $*String
+  %f = function_ref @loadString : $@convention(thin) (@in_guaranteed String) -> @owned String
+  %a = apply %f(%4) : $@convention(thin) (@in_guaranteed String) -> @owned String
+  destroy_addr %4 : $*String
+  dealloc_stack %4 : $*String
+  return %a : $String
+}
+
+// CHECK-LABEL: sil [ossa] @dontExtendAccessScopeOverBeginAccess : $@convention(thin) (@in Klass) -> () {
 // CHECK: bb0(%0 : $*Klass):
-// CHECK:   [[ACCESS:%.*]] = begin_access [read] [static] %0 : $*Klass
+// CHECK:   [[STACK:%.*]] = alloc_stack $Klass
+// CHECK:   [[ACCESS:%.*]] = begin_access [read] [static] [[STACK]] : $*Klass
 // CHECK:   apply %{{.*}}([[ACCESS]]) : $@convention(thin) (@in_guaranteed Klass) -> ()
-// CHECK:   end_access [[ACCESS]] : $*Klass
-// CHECK:   destroy_addr %0 : $*Klass
-// CHECK-LABEL: } // end sil function 'takeWithAccess'
-sil [ossa] @takeWithAccess : $@convention(thin) (@in Klass) -> () {
+// CHECK:   destroy_addr [[STACK]] : $*Klass
+// CHECK-LABEL: } // end sil function 'dontExtendAccessScopeOverBeginAccess'
+sil [ossa] @dontExtendAccessScopeOverBeginAccess : $@convention(thin) (@in Klass) -> () {
 bb0(%0 : $*Klass):
   %stack = alloc_stack $Klass
   %access = begin_access [read] [static] %0 : $*Klass
