Merge pull request #66161 from atrick/fix-silcombine

Fix SILCombine to preserve a release of a move-only type

Author: atrick

lib/SIL/IR/TypeLowering.cpp

@@ -2323,6 +2323,8 @@ namespace {
       return handleReference(classType, properties);
     }
 
+    // WARNING: when the specification of trivial types changes, also update
+    // the isValueTrivial() API used by SILCombine.
     TypeLowering *visitAnyStructType(CanType structType,
                                      AbstractionPattern origType,
                                      StructDecl *D,
@@ -2391,7 +2393,9 @@ namespace {
       return handleAggregateByProperties<LoadableStructTypeLowering>(structType,
                                                                     properties);
     }
-        
+
+    // WARNING: when the specification of trivial types changes, also update
+    // the isValueTrivial() API used by SILCombine.
     TypeLowering *visitAnyEnumType(CanType enumType,
                                    AbstractionPattern origType,
                                    EnumDecl *D,

lib/SILOptimizer/SILCombiner/SILCombinerMiscVisitors.cpp

@@ -815,7 +815,12 @@ SILInstruction *SILCombiner::visitIndexAddrInst(IndexAddrInst *IA) {
 /// operation for \p value is required. This differs from a simple `isTrivial`
 /// check, because it treats a value_to_bridge_object instruction as "trivial".
 /// It can also handle non-trivial enums with trivial cases.
-static bool isTrivial(SILValue value, SILFunction *function) {
+///
+/// TODO: Define this as a top-level SILValue API. It needs to be updated
+/// whenever we refine the specification of isTrivial. For example, in the
+/// future we will check for the existence of a user-defined deinitializer and
+/// handle C++ types specially.
+static bool isValueTrivial(SILValue value, SILFunction *function) {
   SmallVector<ValueBase *, 32> workList;
   SmallPtrSet<ValueBase *, 16> visited;
   workList.push_back(value);
@@ -825,6 +830,11 @@ static bool isTrivial(SILValue value, SILFunction *function) {
       continue;
     if (isa<ValueToBridgeObjectInst>(v))
       continue;
+
+    // MoveOnly types may have a user-defined deinit.
+    if (v->getType().isPureMoveOnly())
+      return false;
+
     if (isa<StructInst>(v) || isa<TupleInst>(v)) {
       for (SILValue op : cast<SingleValueInstruction>(v)->getOperandValues()) {
         if (visited.insert(op).second)
@@ -876,7 +886,7 @@ SILInstruction *SILCombiner::visitReleaseValueInst(ReleaseValueInst *RVI) {
                                        RVI->getAtomicity());
 
   // ReleaseValueInst of a trivial type is a no-op.
-  if (isTrivial(Operand, RVI->getFunction()))
+  if (isValueTrivial(Operand, RVI->getFunction()))
     return eraseInstFromFunction(*RVI);
 
   // Do nothing for non-trivial non-reference types.
@@ -2297,4 +2307,4 @@ SILCombiner::visitCopyAddrInst(CopyAddrInst *CAI) {
   return Builder.createCopyAddr(
       CAI->getLoc(), Src->getOperand(), Dst->getOperand(),
       CAI->isTakeOfSrc(), CAI->isInitializationOfDest());
-}
+}

test/SILOptimizer/sil_combine_moveonly.sil

@@ -0,0 +1,28 @@
+// RUN: %target-sil-opt -enable-sil-verify-all -sil-combine %s | %FileCheck %s
+
+ sil_stage canonical
+
+ import Builtin
+
+ struct S : ~Copyable {
+   deinit {}
+ }
+
+ // Test that a release_value is not removed for a struct-with-deinit.
+ // Doing so would forget the deinit.
+ //
+ // public func testStructDeinit() {
+ //   var s = S()
+ // }
+ //
+ // CHECK-LABEL: sil @testStructDeinit : $@convention(thin) () -> () {
+ // CHECK:         release_value %{{.*}} : $S
+ // CHECK-LABEL: } // end sil function 'testStructDeinit'
+ sil @testStructDeinit : $@convention(thin) () -> () {
+ bb0:
+   %0 = struct $S ()
+   release_value %0 : $S
+   %5 = tuple ()
+   return %5 : $()
+ }
+