Merge pull request #75805 from eeckstein/enable-deinit-devirtualization

Optimizer: enable the DeinitDevirtualizer pass

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/DeinitDevirtualizer.swift

@@ -17,6 +17,10 @@ import SIL
 let deinitDevirtualizer = FunctionPass(name: "deinit-devirtualizer") {
   (function: Function, context: FunctionPassContext) in
 
+  guard function.hasOwnership else {
+    return
+  }
+
   for inst in function.instructions {
     switch inst {
     case let destroyValue as DestroyValueInst:

SwiftCompilerSources/Sources/Optimizer/Utilities/Devirtualization.swift

@@ -39,28 +39,25 @@ private func devirtualize(destroy: some DevirtualizableDestroy, _ context: some
     return true
   }
 
-  let result: Bool
   if type.nominal.hasValueDeinit && !destroy.shouldDropDeinit {
     guard let deinitFunc = context.lookupDeinit(ofNominal: type.nominal) else {
       return false
     }
     destroy.createDeinitCall(to: deinitFunc, context)
-    result = true
-  } else {
-    // If there is no deinit to be called for the original type we have to recursively visit
-    // the struct fields or enum cases.
-    if type.isStruct {
-      result = destroy.devirtualizeStructFields(context)
-    } else if type.isEnum {
-      result = destroy.devirtualizeEnumPayloads(context)
-    } else {
-      precondition(type.isClass, "unknown non-copyable type")
-      // A class reference cannot be further de-composed.
-      return true
-    }
+    context.erase(instruction: destroy)
+    return true
   }
-  context.erase(instruction: destroy)
-  return result
+  // If there is no deinit to be called for the original type we have to recursively visit
+  // the struct fields or enum cases.
+  if type.isStruct {
+    return destroy.devirtualizeStructFields(context)
+  }
+  if type.isEnum {
+    return destroy.devirtualizeEnumPayloads(context)
+  }
+  precondition(type.isClass, "unknown non-copyable type")
+  // A class reference cannot be further de-composed.
+  return true
 }
 
 // Used to dispatch devirtualization tasks to `destroy_value` and `destroy_addr`.
@@ -79,6 +76,10 @@ private extension DevirtualizableDestroy {
     guard let cases = type.getEnumCases(in: parentFunction) else {
       return false
     }
+    defer {
+      context.erase(instruction: self)
+    }
+
     if cases.allPayloadsAreTrivial(in: parentFunction) {
       let builder = Builder(before: self, context)
       builder.createEndLifetime(of: operand.value)
@@ -122,11 +123,15 @@ extension DestroyValueInst : DevirtualizableDestroy {
   }
 
   fileprivate func devirtualizeStructFields(_ context: some MutatingContext) -> Bool {
-    let builder = Builder(before: self, context)
-
     guard let fields = type.getNominalFields(in: parentFunction) else {
       return false
     }
+
+    defer {
+      context.erase(instruction: self)
+    }
+
+    let builder = Builder(before: self, context)
     if fields.allFieldsAreTrivial(in: parentFunction) {
       builder.createEndLifetime(of: operand.value)
       return true
@@ -193,6 +198,9 @@ extension DestroyAddrInst : DevirtualizableDestroy {
     guard let fields = type.getNominalFields(in: parentFunction) else {
       return false
     }
+    defer {
+      context.erase(instruction: self)
+    }
     if fields.allFieldsAreTrivial(in: parentFunction) {
       builder.createEndLifetime(of: operand.value)
       return true

include/swift/SIL/SILBridgingImpl.h

@@ -1615,7 +1615,7 @@ BridgedInstruction BridgedBuilder::createAllocStack(BridgedType type,
       regularLoc(), type.unbridged(), std::nullopt,
       swift::HasDynamicLifetime_t(hasDynamicLifetime),
       swift::IsLexical_t(isLexical), swift::IsFromVarDecl_t(isFromVarDecl),
-      swift::UsesMoveableValueDebugInfo_t(wasMoved))};
+      swift::UsesMoveableValueDebugInfo_t(wasMoved), /*skipVarDeclAssert=*/ true)};
 }
 
 BridgedInstruction BridgedBuilder::createAllocVector(BridgedValue capacity, BridgedType type) const {

lib/SILOptimizer/FunctionSignatureTransforms/OwnedToGuaranteedTransform.cpp

@@ -80,6 +80,13 @@ bool FunctionSignatureTransform::OwnedToGuaranteedAnalyzeParameters() {
     if (!A.canOptimizeLiveArg()) {
       continue;
     }
+    if (A.Arg->getType().isMoveOnly()) {
+      // We must not do this transformation for non-copyable types, because it's
+      // not safe to insert a compensating release_value at the call site. This
+      // release_value calls the deinit which might have been already de-
+      // virtualized in the callee.
+      continue;
+    }
 
     // See if we can find a ref count equivalent strong_release or release_value
     // at the end of this function if our argument is an @owned parameter.

lib/SILOptimizer/PassManager/PassPipeline.cpp

@@ -62,10 +62,6 @@ static llvm::cl::opt<bool> SILViewSILGenCFG(
     "sil-view-silgen-cfg", llvm::cl::init(false),
     llvm::cl::desc("Enable the sil cfg viewer pass before diagnostics"));
 
-static llvm::cl::opt<bool>
-    EnableDeinitDevirtualizer("enable-deinit-devirtualizer", llvm::cl::init(false),
-                          llvm::cl::desc("Enable the DestroyHoisting pass."));
-
 //===----------------------------------------------------------------------===//
 //                          Diagnostic Pass Pipeline
 //===----------------------------------------------------------------------===//
@@ -191,14 +187,6 @@ static void addMandatoryDiagnosticOptPipeline(SILPassPipelinePlan &P) {
     P.addLifetimeDependenceDiagnostics();
   }
 
-  // Devirtualize deinits early if requested.
-  //
-  // FIXME: why is DeinitDevirtualizer in the middle of the mandatory pipeline,
-  // and what passes/compilation modes depend on it? This pass is never executed
-  // or tested without '-Xllvm enable-deinit-devirtualizer'.
-  if (EnableDeinitDevirtualizer)
-    P.addDeinitDevirtualizer();
-
   // As a temporary measure, we also eliminate move only for non-trivial types
   // until we can audit the later part of the pipeline. Eventually, this should
   // occur before IRGen.
@@ -518,6 +506,8 @@ void addFunctionPasses(SILPassPipelinePlan &P,
   }
   P.addARCSequenceOpts();
 
+  P.addDeinitDevirtualizer();
+
   // We earlier eliminated ownership if we are not compiling the stdlib. Now
   // handle the stdlib functions, re-simplifying, eliminating ARC as we do.
   if (P.getOptions().CopyPropagation != CopyPropagationOption::Off) {

test/IRGen/moveonly_value_functions.swift

@@ -1,4 +1,5 @@
 // RUN: %target-swift-emit-irgen -O                          \
+// RUN:     -Xllvm -sil-disable-pass=deinit-devirtualizer    \
 // RUN:     -disable-type-layout                             \
 // RUN:     %s                                               \
 // RUN: |                                                    \

test/IRGen/raw_layout_multifile.swift

@@ -1,6 +1,6 @@
 // RUN: %empty-directory(%t)
 // RUN: %{python} %utils/chex.py < %s > %t/raw_layout_multifile.swift
-// RUN: %target-swift-frontend -enable-experimental-feature BuiltinModule -enable-experimental-feature RawLayout -emit-ir -O -disable-availability-checking %s %S/Inputs/raw_layout_multifile_b.swift | %FileCheck %t/raw_layout_multifile.swift --check-prefix=CHECK --check-prefix=CHECK-%target-ptrsize
+// RUN: %target-swift-frontend -enable-experimental-feature BuiltinModule -enable-experimental-feature RawLayout -emit-ir -O -Xllvm -sil-disable-pass=deinit-devirtualizer -disable-availability-checking %s %S/Inputs/raw_layout_multifile_b.swift | %FileCheck %t/raw_layout_multifile.swift --check-prefix=CHECK --check-prefix=CHECK-%target-ptrsize
 
 @_rawLayout(like: Int32)
 public struct Foo<T>: ~Copyable {}

test/Interpreter/moveonly_swiftskell.swift

@@ -17,6 +17,11 @@
 // RUN: %target-run %t/E %t/%target-library-name(Swiftskell) \
 // RUN:     | %FileCheck %s --implicit-check-not destroy
 
+// RUN: %target-build-swift -O -I%t -L%t -lSwiftskell -parse-as-library %s \
+// RUN:                     -module-name Opt -o %t/Opt %target-rpath(%t)
+// RUN: %target-codesign %t/Opt
+// RUN: %target-run %t/Opt %t/%target-library-name(Swiftskell) | %FileCheck %s --implicit-check-not destroy
+
 // REQUIRES: executable_test
 
 // Temporarily disable for back-deployment (rdar://128544927)

test/SILOptimizer/devirt_deinits.sil

@@ -283,6 +283,16 @@ bb0(%0 : $*T):
   return %r : $()
 }
 
+// CHECK-LABEL: sil @test_non_ossa :
+// CHECK:         destroy_addr %0
+// CHECK:       } // end sil function 'test_non_ossa'
+sil @test_non_ossa : $@convention(thin) (@in S1) -> () {
+bb0(%0 : $*S1):
+  destroy_addr %0 : $*S1
+  %r = tuple()
+  return %r : $()
+}
+
 sil @s1_deinit : $@convention(method) (@owned S1) -> ()
 sil @s2_deinit : $@convention(method) (@owned S2) -> ()
 sil @s3_deinit : $@convention(method) <T> (@in S3<T>) -> ()

test/SILOptimizer/devirt_deinits.swift

@@ -1,7 +1,7 @@
-// RUN: %target-swift-frontend -target %target-cpu-apple-macos14 -primary-file %s -parse-as-library -sil-verify-all -Xllvm -enable-deinit-devirtualizer -module-name=test -emit-sil | %FileCheck %s
+// RUN: %target-swift-frontend -target %target-cpu-apple-macos14 -primary-file %s -parse-as-library -O -sil-verify-all -module-name=test -Xllvm -sil-disable-pass=function-signature-opts -emit-sil | %FileCheck %s
 
 // Also do an end-to-end test and check if the compiled executable works as expected.
-// RUN: %target-run-simple-swift(-target %target-cpu-apple-macos14 -Xllvm -enable-deinit-devirtualizer -parse-as-library) | %FileCheck -check-prefix CHECK-OUTPUT %s
+// RUN: %target-run-simple-swift(-target %target-cpu-apple-macos14 -parse-as-library -O -Xllvm -sil-disable-pass=function-signature-opts) | %FileCheck -check-prefix CHECK-OUTPUT %s
 
 // Check if it works in embedded mode.
 // RUN: %target-run-simple-swift(-target %target-cpu-apple-macos14 -enable-experimental-feature Embedded -parse-as-library -runtime-compatibility-version none -wmo -Xfrontend -disable-objc-interop) | %FileCheck -check-prefix CHECK-OUTPUT %s
@@ -98,8 +98,8 @@ func testTwoFieldDeinits(_ s: consuming StrWithoutDeinit) {
 }
 
 // CHECK-LABEL: sil hidden [noinline] @$s4test0A5Enum1yyAA2E1OnF :
-// CHECK:         switch_enum_addr
-// CHECK:       bb1:
+// CHECK:         switch_enum
+// CHECK:       bb1({{.*}}):
 // CHECK:         [[D:%.*]] = function_ref @$s4test2S1VfD :
 // CHECK:         apply [[D]]
 // CHECK:       } // end sil function '$s4test0A5Enum1yyAA2E1OnF'
@@ -109,13 +109,13 @@ func testEnum1(_ s: consuming E1) {
 }
 
 // CHECK-LABEL: sil hidden [noinline] @$s4test0A5Enum2yyAA2E2OnF :
-// CHECK:         switch_enum_addr
-// CHECK:       bb1:
+// CHECK:         switch_enum
+// CHECK:       bb1({{.*}}):
 // CHECK:         [[D:%.*]] = function_ref @$s4test2S2VfD :
 // CHECK:         apply [[D]]
-// CHECK:       bb2:
-// CHECK:         switch_enum_addr
-// CHECK:       bb3:
+// CHECK:       bb2({{.*}}):
+// CHECK:         switch_enum
+// CHECK:       bb3({{.*}}):
 // CHECK:         [[D:%.*]] = function_ref @$s4test2S1VfD :
 // CHECK:         apply [[D]]
 // CHECK:       } // end sil function '$s4test0A5Enum2yyAA2E2OnF'

test/SILOptimizer/stdlib/Atomics.swift

@@ -15,8 +15,7 @@ func testInt(_: Int)
 // CHECK:         [[ATOMIC:%.*]] = alloc_stack [lexical] [var_decl] $Atomic<Int>
 // CHECK:         [[ATOMIC_PTR:%.*]] = address_to_pointer [[ATOMIC]]
 // CHECK:         builtin "atomicload_monotonic_Int[[PTR_SIZE]]"([[ATOMIC_PTR]] : $Builtin.RawPointer)
-// CHECK:         destroy_addr [[ATOMIC]] : $*Atomic<Int>
-// CHECK-NEXT:    dealloc_stack [[ATOMIC]] : $*Atomic<Int>
+// CHECK:         dealloc_stack [[ATOMIC]] : $*Atomic<Int>
 // CHECK-LABEL: } // end sil function 'localLoad'
 @_silgen_name("localLoad")
 func localLoad() -> Int {
@@ -28,8 +27,7 @@ func localLoad() -> Int {
 // CHECK:         [[ATOMIC:%.*]] = alloc_stack [lexical] [var_decl] $Atomic<Int>
 // CHECK:         [[ATOMIC_PTR:%.*]] = address_to_pointer [[ATOMIC]]
 // CHECK:         builtin "atomicstore_release_Int[[PTR_SIZE]]"([[ATOMIC_PTR]] : $Builtin.RawPointer
-// CHECK:         destroy_addr [[ATOMIC]] : $*Atomic<Int>
-// CHECK-NEXT:    dealloc_stack [[ATOMIC]] : $*Atomic<Int>
+// CHECK:         dealloc_stack [[ATOMIC]] : $*Atomic<Int>
 // CHECK-LABEL: } // end sil function 'localStore'
 @_silgen_name("localStore")
 func localStore() {
@@ -41,8 +39,7 @@ func localStore() {
 // CHECK:         [[ATOMIC:%.*]] = alloc_stack [lexical] [var_decl] $Atomic<Int>
 // CHECK:         [[ATOMIC_PTR:%.*]] = address_to_pointer [[ATOMIC]]
 // CHECK:         builtin "atomicrmw_xchg_acquire_Int[[PTR_SIZE]]"([[ATOMIC_PTR]] : $Builtin.RawPointer
-// CHECK:         destroy_addr [[ATOMIC]] : $*Atomic<Int>
-// CHECK-NEXT:    dealloc_stack [[ATOMIC]] : $*Atomic<Int>
+// CHECK:         dealloc_stack [[ATOMIC]] : $*Atomic<Int>
 // CHECK-LABEL: } // end sil function 'localExchange'
 @_silgen_name("localExchange")
 func localExchange() -> Int {
@@ -54,8 +51,7 @@ func localExchange() -> Int {
 // CHECK:         [[ATOMIC:%.*]] = alloc_stack [lexical] [var_decl] $Atomic<Int>
 // CHECK:         [[ATOMIC_PTR:%.*]] = address_to_pointer [[ATOMIC]]
 // CHECK:         builtin "cmpxchg_seqcst_seqcst_Int[[PTR_SIZE]]"([[ATOMIC_PTR]] : $Builtin.RawPointer
-// CHECK:         destroy_addr [[ATOMIC]] : $*Atomic<Int>
-// CHECK-NEXT:    dealloc_stack [[ATOMIC]] : $*Atomic<Int>
+// CHECK:         dealloc_stack [[ATOMIC]] : $*Atomic<Int>
 // CHECK-LABEL: } // end sil function 'localCompareExchange'
 @_silgen_name("localCompareExchange")
 func localCompareExchange() -> (exchanged: Bool, original: Int) {

test/embedded/non-copyable-deinit.swift

@@ -0,0 +1,28 @@
+// RUN: %target-run-simple-swift(-enable-experimental-feature Embedded -O -parse-as-library -runtime-compatibility-version none -wmo -Xfrontend -disable-objc-interop) | %FileCheck %s
+
+// REQUIRES: executable_test
+// REQUIRES: OS=macosx || OS=linux-gnu
+
+var deinitCalled = false
+
+struct S: ~Copyable {
+  let s: String
+
+  @inline(never)
+  deinit {
+    precondition(!deinitCalled)
+    deinitCalled = true
+    print(s)
+  }
+}
+
+@main
+struct Main {
+  static func main() {
+    // CHECK:      1
+    _ = S(s: "1")
+    // CHECK-NEXT: done
+    print("done")
+  }
+}
+