Merge pull request #23431 from eeckstein/fix-cowarrayopt-5.1

[5.1] SILOptimier: Fix a miscompile in COWArrayOpt.

Author: eeckstein

lib/SILOptimizer/LoopTransforms/COWArrayOpt.cpp

@@ -410,8 +410,7 @@ class COWArrayOpt {
   // analyzing.
   SILValue CurrentArrayAddr;
 public:
-  COWArrayOpt(RCIdentityFunctionInfo *RCIA, SILLoop *L,
-              DominanceAnalysis *DA)
+  COWArrayOpt(RCIdentityFunctionInfo *RCIA, SILLoop *L, DominanceAnalysis *DA)
       : RCIA(RCIA), Function(L->getHeader()->getParent()), Loop(L),
         Preheader(L->getLoopPreheader()), DomTree(DA->get(Function)),
         ColdBlocks(DA), CachedSafeLoop(false, false) {}
@@ -427,7 +426,8 @@ class COWArrayOpt {
   bool checkSafeArrayValueUses(UserList &ArrayValueUsers);
   bool checkSafeArrayElementUse(SILInstruction *UseInst, SILValue ArrayVal);
   bool checkSafeElementValueUses(UserOperList &ElementValueUsers);
-  bool hoistMakeMutable(ArraySemanticsCall MakeMutable);
+  bool hoistMakeMutable(ArraySemanticsCall MakeMutable, bool dominatesExits);
+  bool dominatesExitingBlocks(SILBasicBlock *BB);
   void hoistAddressProjections(Operand &ArrayOp);
   bool hasLoopOnlyDestructorSafeArrayOperations();
   SILValue getArrayAddressBase(SILValue V);
@@ -1037,7 +1037,8 @@ void COWArrayOpt::hoistAddressProjections(Operand &ArrayOp) {
 
 /// Check if this call to "make_mutable" is hoistable, and move it, or delete it
 /// if it's already hoisted.
-bool COWArrayOpt::hoistMakeMutable(ArraySemanticsCall MakeMutable) {
+bool COWArrayOpt::hoistMakeMutable(ArraySemanticsCall MakeMutable,
+                                   bool dominatesExits) {
   LLVM_DEBUG(llvm::dbgs() << "    Checking mutable array: " <<CurrentArrayAddr);
 
   // We can hoist address projections (even if they are only conditionally
@@ -1056,7 +1057,12 @@ bool COWArrayOpt::hoistMakeMutable(ArraySemanticsCall MakeMutable) {
 
   // Check whether we can hoist make_mutable based on the operations that are
   // in the loop.
-  if (hasLoopOnlyDestructorSafeArrayOperations()) {
+  // Note that in this case we don't verify that the array buffer is not aliased
+  // and therefore we must be conservative if the make_mutable is executed
+  // conditionally (i.e. doesn't dominate all exit blocks).
+  // The test SILOptimizer/cowarray_opt.sil: dont_hoist_if_executed_conditionally
+  // shows the problem.
+  if (hasLoopOnlyDestructorSafeArrayOperations() && dominatesExits) {
     // Done. We can hoist the make_mutable.
     // We still need the array uses later to check if we can add loads to
     // HoistableLoads.
@@ -1106,6 +1112,16 @@ bool COWArrayOpt::hoistMakeMutable(ArraySemanticsCall MakeMutable) {
   return true;
 }
 
+bool COWArrayOpt::dominatesExitingBlocks(SILBasicBlock *BB) {
+  llvm::SmallVector<SILBasicBlock *, 8> ExitingBlocks;
+  Loop->getExitingBlocks(ExitingBlocks);
+  for (SILBasicBlock *Exiting : ExitingBlocks) {
+    if (!DomTree->dominates(BB, Exiting))
+      return false;
+  }
+  return true;
+}
+
 bool COWArrayOpt::run() {
   LLVM_DEBUG(llvm::dbgs() << "  Array Opts in Loop " << *Loop);
 
@@ -1123,6 +1139,7 @@ bool COWArrayOpt::run() {
   for (auto *BB : Loop->getBlocks()) {
     if (ColdBlocks.isCold(BB))
       continue;
+    bool dominatesExits = dominatesExitingBlocks(BB);
     for (auto II = BB->begin(), IE = BB->end(); II != IE;) {
       // Inst may be moved by hoistMakeMutable.
       SILInstruction *Inst = &*II;
@@ -1134,7 +1151,7 @@ bool COWArrayOpt::run() {
       CurrentArrayAddr = MakeMutableCall.getSelf();
       auto HoistedCallEntry = ArrayMakeMutableMap.find(CurrentArrayAddr);
       if (HoistedCallEntry == ArrayMakeMutableMap.end()) {
-        if (!hoistMakeMutable(MakeMutableCall)) {
+        if (!hoistMakeMutable(MakeMutableCall, dominatesExits)) {
           ArrayMakeMutableMap[CurrentArrayAddr] = nullptr;
           continue;
         }

test/SILOptimizer/cowarray_opt.sil

@@ -195,6 +195,39 @@ bb2:
   return %7 : $()
 }
 
+// CHECK-LABEL: sil @dont_hoist_if_executed_conditionally
+// CHECK: bb0({{.*}}):
+// CHECK-NOT: apply
+// CHECK: bb1({{.*}}):
+// CHECK: bb2:
+// CHECK:   apply
+// CHECK: bb3:
+sil @dont_hoist_if_executed_conditionally : $@convention(thin) (@inout MyArray<MyStruct>, @inout Builtin.Int1) -> MyArray<MyStruct> {
+bb0(%0 : $*MyArray<MyStruct>, %1 : $*Builtin.Int1):
+  debug_value_addr %0 : $*MyArray<MyStruct>
+  %2 = load %0 : $*MyArray<MyStruct>
+  br bb1(%2 : $MyArray<MyStruct>)
+
+bb1(%p1 : $MyArray<MyStruct>):
+  cond_br undef, bb2, bb3
+
+bb2:
+  // If this block is never taken, then hoisting to bb0 would change the value of %p3.
+  %5 = function_ref @array_make_mutable : $@convention(method) (@inout MyArray<MyStruct>) -> ()
+  %6 = apply %5(%0) : $@convention(method) (@inout MyArray<MyStruct>) -> ()
+  %7 = load %0 : $*MyArray<MyStruct>
+  br bb4(%7 : $MyArray<MyStruct>)
+
+bb3:
+  br bb4(%p1 : $MyArray<MyStruct>)
+
+bb4(%p2 : $MyArray<MyStruct>):
+  cond_br undef, bb1(%p2 : $MyArray<MyStruct>), bb5(%p2 : $MyArray<MyStruct>)
+
+bb5(%p3 : $MyArray<MyStruct>):
+  return %p3 : $MyArray<MyStruct>
+}
+
 // CHECK-LABEL: sil @cow_should_ignore_mark_dependence_addrproj_use : $@convention(thin) (@inout MyArray<MyStruct>, @inout Builtin.Int1) -> () {
 // CHECK: bb0(
 // CHECK-NOT: br bb

test/SILOptimizer/cowarray_opt_crash.swift

@@ -0,0 +1,35 @@
+// RUN: %empty-directory(%t) 
+// RUN: %target-build-swift -O %s -o %t/a.out
+// RUN: %target-run %t/a.out | %FileCheck %s
+
+// REQUIRES: executable_test
+
+// Check that the compiled code does not crash because of an over-release.
+// End-to-end test for rdar://problem/48906146.
+
+struct LCRNG: RandomNumberGenerator {
+  private var state: UInt64
+
+  init(seed: Int) {
+    state = UInt64(truncatingIfNeeded: seed)
+    for _ in 0..<10 { _ = next() }
+  }
+
+  mutating func next() -> UInt64 {
+    state = 2862933555777941757 &* state &+ 3037000493
+    return state
+  }
+}
+
+
+func test(_ body: () -> Void) {
+  body()
+}
+
+test {
+  var rng = LCRNG(seed: 42)
+  let v = Array(0..<3).shuffled(using: &rng)
+  // CHECK: 3
+  print(v.count)
+}
+