Merge pull request #28989 from gottesmm/pr-42b06814f186ae88911b84b21640d36b25c7a1cd

[semantic-arc-opts] When performing load [copy] -> load_borrow on classes, do not ignore forwarding uses.

Author: gottesmm

lib/SILOptimizer/Mandatory/SemanticARCOpts.cpp

@@ -36,6 +36,25 @@ STATISTIC(NumEliminatedInsts, "number of removed instructions");
 STATISTIC(NumLoadCopyConvertedToLoadBorrow,
           "number of load_copy converted to load_borrow");
 
+//===----------------------------------------------------------------------===//
+//                                  Utility
+//===----------------------------------------------------------------------===//
+
+static bool isConsumingOwnedUse(Operand *use) {
+  assert(use->get().getOwnershipKind() == ValueOwnershipKind::Owned);
+
+  if (use->isTypeDependent())
+    return false;
+
+  // We know that a copy_value produces an @owned value. Look through all of
+  // our uses and classify them as either invalidating or not
+  // invalidating. Make sure that all of the invalidating ones are
+  // destroy_value since otherwise the live_range is not complete.
+  auto map = use->getOwnershipKindMap();
+  auto constraint = map.getLifetimeConstraint(ValueOwnershipKind::Owned);
+  return constraint == UseLifetimeConstraint::MustBeInvalidated;
+}
+
 //===----------------------------------------------------------------------===//
 //                            Live Range Modeling
 //===----------------------------------------------------------------------===//
@@ -775,8 +794,21 @@ class StorageGuaranteesLoadVisitor
                std::back_inserter(baseEndBorrows));
 
     SmallVector<SILInstruction *, 4> valueDestroys;
-    llvm::copy(Load->getUsersOfType<DestroyValueInst>(),
-               std::back_inserter(valueDestroys));
+    for (auto *use : Load->getUses()) {
+      // If this load is not consuming, skip it.
+      if (!isConsumingOwnedUse(use)) {
+        continue;
+      }
+
+      // Otherwise, if this isn't a destroy_value, we have a consuming use we
+      // don't understand. Return conservatively that this memory location may
+      // not be guaranteed.
+      auto *user = use->getUser();
+      if (!isa<DestroyValueInst>(user)) {
+        return answer(true);
+      }
+      valueDestroys.emplace_back(user);
+    }
 
     SmallPtrSet<SILBasicBlock *, 4> visitedBlocks;
     LinearLifetimeChecker checker(visitedBlocks, ARCOpt.getDeadEndBlocks());

test/SILOptimizer/semantic-arc-opts.sil

@@ -9,6 +9,10 @@ import Builtin
 //////////////////
 
 enum MyNever {}
+enum FakeOptional<T> {
+case none
+case some(T)
+}
 
 sil @guaranteed_user : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
 sil @owned_user : $@convention(thin) (@owned Builtin.NativeObject) -> ()
@@ -24,6 +28,7 @@ sil @get_nativeobject_pair : $@convention(thin) () -> @owned NativeObjectPair
 
 class Klass {}
 sil @guaranteed_klass_user : $@convention(thin) (@guaranteed Klass) -> ()
+sil @guaranteed_fakeoptional_klass_user : $@convention(thin) (@guaranteed FakeOptional<Klass>) -> ()
 
 struct MyInt {
   var value: Builtin.Int32
@@ -40,11 +45,6 @@ struct StructMemberTest {
   var t : (Builtin.Int32, AnotherStruct)
 }
 
-enum FakeOptional<T> {
-case none
-case some(T)
-}
-
 class ClassLet {
   @_hasStorage let aLet: Klass
   @_hasStorage var aVar: Klass
@@ -589,6 +589,52 @@ bb0(%x : @owned $ClassLet):
   return undef : $()
 }
 
+// We do not support this today, but we will once forwarding is ignored when
+// checking if the load [copy] is a dead live range.
+//
+// CHECK-LABEL: sil [ossa] @dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase :
+// CHECK: load [copy]
+// CHECK: } // end sil function 'dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase'
+sil [ossa] @dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase : $@convention(thin) (@owned ClassLet) -> () {
+bb0(%x : @owned $ClassLet):
+  %f = function_ref @black_hole : $@convention(thin) (@guaranteed Klass) -> ()
+
+  %a = begin_borrow %x : $ClassLet
+  %p = ref_element_addr %a : $ClassLet, #ClassLet.aLetTuple
+  %v = load [copy] %p : $*(Klass, Klass)
+  (%v1, %v2) = destructure_tuple %v : $(Klass, Klass)
+  apply %f(%v1) : $@convention(thin) (@guaranteed Klass) -> ()
+  apply %f(%v2) : $@convention(thin) (@guaranteed Klass) -> ()
+  destroy_value %v1 : $Klass
+  destroy_value %v2 : $Klass
+  end_borrow %a : $ClassLet
+  destroy_value %x : $ClassLet
+
+  return undef : $()
+}
+
+// We do not support this today, but we will once forwarding is ignored when
+// checking if the load [copy] is a dead live range.
+//
+// CHECK-LABEL: sil [ossa] @dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase_2 :
+// CHECK: load [copy]
+// CHECK: } // end sil function 'dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase_2'
+sil [ossa] @dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase_2 : $@convention(thin) (@owned ClassLet) -> () {
+bb0(%x : @owned $ClassLet):
+  %f = function_ref @guaranteed_user : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
+
+  %a = begin_borrow %x : $ClassLet
+  %p = ref_element_addr %a : $ClassLet, #ClassLet.aLet
+  %v = load [copy] %p : $*Klass
+  %v_cast = unchecked_ref_cast %v : $Klass to $Builtin.NativeObject
+  apply %f(%v_cast) : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
+  destroy_value %v_cast : $Builtin.NativeObject
+  end_borrow %a : $ClassLet
+  destroy_value %x : $ClassLet
+
+  return undef : $()
+}
+
 // CHECK-LABEL: sil [ossa] @dont_copy_let_properties_with_multi_borrowed_base_that_dominates
 // CHECK:         [[OUTER:%.*]] = begin_borrow
 // CHECK-NEXT:    ref_element_addr
@@ -662,7 +708,7 @@ bb0(%x : @owned $ClassLet):
   return undef : $()
 }
 
-// CHECK-LABEL: sil [ossa] @do_or_dont_copy_let_properties_with_multi_borrowed_base_when_it_dominates
+// CHECK-LABEL: sil [ossa] @do_or_dont_copy_let_properties_with_multi_borrowed_base_when_it_dominates_2 :
 // CHECK:         [[OUTER:%.*]] = begin_borrow
 // CHECK-NEXT:    ref_element_addr
 // CHECK-NEXT:    [[INNER:%.*]] = load_borrow
@@ -672,12 +718,15 @@ bb0(%x : @owned $ClassLet):
 // CHECK-NEXT:    begin_borrow
 // CHECK-NEXT:    ref_element_addr
 // CHECK-NEXT:    load [copy]
-// CHECK-NEXT:    apply
 // CHECK-NEXT:    end_borrow
 // CHECK-NEXT:    destroy_value
+// CHECK-NEXT:    // function_ref
+// CHECK-NEXT:    function_ref
+// CHECK-NEXT:    enum
 // CHECK-NEXT:    apply
 // CHECK-NEXT:    destroy_value
-sil [ossa] @do_or_dont_copy_let_properties_with_multi_borrowed_base_when_it_dominates : $@convention(thin) (@owned ClassLet) -> () {
+// CHECK: } // end sil function 'do_or_dont_copy_let_properties_with_multi_borrowed_base_when_it_dominates_2'
+sil [ossa] @do_or_dont_copy_let_properties_with_multi_borrowed_base_when_it_dominates_2 : $@convention(thin) (@owned ClassLet) -> () {
 bb0(%x : @owned $ClassLet):
   %f = function_ref @black_hole : $@convention(thin) (@guaranteed Klass) -> ()
 
@@ -693,18 +742,17 @@ bb0(%x : @owned $ClassLet):
   %b = begin_borrow %x : $ClassLet
   %q = ref_element_addr %b : $ClassLet, #ClassLet.aLet
   %w = load [copy] %q : $*Klass
-  %d = begin_borrow %w : $Klass
-  apply %f(%d) : $@convention(thin) (@guaranteed Klass) -> ()
 
   // End the lifetime of the base object first...
   end_borrow %b : $ClassLet
   destroy_value %x : $ClassLet
 
   // ...then end the lifetime of the copy.
-  apply %f(%d) : $@convention(thin) (@guaranteed Klass) -> ()
+  %f2 = function_ref @guaranteed_fakeoptional_klass_user : $@convention(thin) (@guaranteed FakeOptional<Klass>) -> ()
+  %w2 = enum $FakeOptional<Klass>, #FakeOptional.some!enumelt.1, %w : $Klass
+  apply %f2(%w2) : $@convention(thin) (@guaranteed FakeOptional<Klass>) -> ()
 
-  end_borrow %d : $Klass
-  destroy_value %w : $Klass
+  destroy_value %w2 : $FakeOptional<Klass>
 
   return undef : $()
 }