[DCE] Tweaked code to end borrows before destroys.

If a phi argument is dead and it reborrowing it was dependent on some
other value, that other value on which it was dependent may have already
itself been deleted.  In that case, the destroy_value would have been
added just before the terminator of the predecessors of the block which
contained the dead phi.  So, when deciding where to insert the
end_borrow, iterate backwards from the end of the block, skipping the
terminator updating the insertion point every time a destroy_value
instruction is encountered until we hit an instruction with a different
opcode.  This ensures that no matter how many destroy_values may have
been added just before the terminator, the end_borrow will preceed them.

This commit just tweaks the preexisting logic that checked for this
condition.  Specifically, the previous code didn't handle the case where
the block contains only a terminator and a destroy_value.

Author: nate-chandler

lib/SILOptimizer/Transforms/DeadCodeElimination.cpp

@@ -601,13 +601,19 @@ bool DCE::removeDead() {
           // may also have been dead and a destroy_value of its baseValue may
           // have been inserted before the pred's terminator. Make sure to
           // adjust the insertPt before any destroy_value.
+          //
+          // FIXME: This code currently can reorder destroys, e.g., when the
+          //        block already contains a destroy_value just before the
+          //        terminator.  Fix this by making note of the added
+          //        destroy_value insts and only moving the insertion point
+          //        before those that are newly added.
           for (SILInstruction &predInst : llvm::reverse(*pred)) {
             if (&predInst == predTerm)
               continue;
             if (!isa<DestroyValueInst>(&predInst)) {
-              insertPt = &*std::next(predInst.getIterator());
               break;
             }
+            insertPt = &predInst;
           }
         }
 

test/SILOptimizer/dead_code_elimination_nontrivial_ossa.sil

@@ -679,3 +679,44 @@ exit:
   %retval = tuple ()
   return %retval : $()
 }
+
+// CHECK-LABEL: sil hidden [ossa] @add_end_borrow_after_destroy_value : {{.*}} {
+// CHECK:       bb0({{%[^,]+}} : $Builtin.Int1, [[INSTANCE_1:%[^,]+]] : @owned $Klass, {{%[^,]+}} : @owned $Klass):
+// CHECK:         [[LIFETIME_1:%[^,]+]] = begin_borrow [[INSTANCE_1]]
+// CHECK:         copy_value [[LIFETIME_1]]
+// CHECK:         cond_br {{%[^,]+}}, [[BASIC_BLOCK1:bb[0-9]+]], [[BASIC_BLOCK2:bb[0-9]+]]
+// CHECK:       [[BASIC_BLOCK1]]:
+// CHECK:         end_borrow [[LIFETIME_1]]
+// CHECK:         destroy_value [[INSTANCE_1]]
+// CHECK:       [[BASIC_BLOCK2]]:
+// CHECK:         end_borrow [[LIFETIME_1]]
+// CHECK:         destroy_value [[INSTANCE_1]]
+// CHECK-LABEL: } // end sil function 'add_end_borrow_after_destroy_value'
+sil hidden [ossa] @add_end_borrow_after_destroy_value : $@convention(thin) (Builtin.Int1, @owned Klass, @owned Klass) -> () {
+bb0(%condition : $Builtin.Int1, %instance_1 : @owned $Klass, %instance_2 : @owned $Klass):
+  %lifetime_1 = begin_borrow %instance_1 : $Klass
+  
+  %copy_1 = copy_value %lifetime_1 : $Klass
+  %stack_addr = alloc_stack $Klass
+  store %copy_1 to [init] %stack_addr : $*Klass
+  destroy_addr %stack_addr : $*Klass
+  dealloc_stack %stack_addr : $*Klass
+
+  cond_br %condition, bb1, bb2
+
+bb1:
+  end_borrow %lifetime_1 : $Klass
+  destroy_value %instance_1 : $Klass
+  %lifetime_2 = begin_borrow %instance_2 : $Klass
+  br bb3(%instance_2 : $Klass, %lifetime_2 : $Klass)
+
+bb2:
+  destroy_value %instance_2 : $Klass
+  br bb3(%instance_1 : $Klass, %lifetime_1 : $Klass)
+
+bb3(%original : @owned $Klass, %lifetime : @guaranteed $Klass):
+  end_borrow %lifetime : $Klass
+  destroy_value %original : $Klass
+  %result = tuple ()
+  return %result : $()
+}