Optimizer: make destroy hoisting a mandatory pass

[eeckstein]

So far hoisting destroys (destroy_value and destroy_addr) in the optimizer worked differently for lexical and non-lexical lifetimes: lexical lifetimes need to respect deinit-barriers, while destroys of non-lexical lifetimes could be moved over deinit-barriers.

This had two problems:

• The behavior of a compiled program can be different between Onone and optimized builds.
• It is complicated to correctly maintain the "is-lexical" flags in SIL throughout the optimizer pipeline. Even worse, some optimization do not run in cases where it's difficult to maintain the flags.

This PR

• introduces a new MandatoryDestroyHoisting pass which hoists destroys of non-lexical lifetimes in the mandatory pipeline
• makes all destroy-hoisting passes in the optimizer respect deinit-barriers for all lifetimes, including non-lexical lifetimes.

This almost guarantees consistent behavior for Onone and optimized builds. Almost, because there is still one exception for COW types (like Arrays). Such values are not lexical, but MandatoryDestroyHoisting does not hoist destroys of such variables in debug builds to not compromise debug info.

[eeckstein]

@swift-ci test

[eeckstein]

@swift-ci apple silicon benchmark

[rjmccall]

The goal here is to just provide this optimization at -Onone rather than to guarantee some semantic effect? So it's okay that it doesn't run in bootstrap.

[meg-gupta]

Curious why you saw this

The behavior of a compiled program can be different between Onone and optimized builds.

Non lexical lifetimes should not have observable program behavior. Was this because we did not correctly mark a value as lexical or lost this information during transformations? Is there a test showing this?

[eeckstein]

The goal here is to just provide this optimization at -Onone rather than to guarantee some semantic effect? So it's okay that it doesn't run in bootstrap.

right

[eeckstein]

Non lexical lifetimes should not have observable program behavior.

They do. For example, a temporary class reference can have a side effect in it's deinit. If such a temporary value is moved over a deinit barrier, it's changing the program behavior.

[eeckstein]

@swift-ci test macos

[eeckstein]

@swift-ci test windows

[eeckstein]

@swift-ci test Windows

[eeckstein]

@swift-ci smoke test Windows

[atrick]

For diagnostics, we need to concept of "this value's lifetime represents the scope of a variable declaration". We should have a common way of checking for that. We have Instruction.findVarDecl(). So for DestroyHoisting at -Onone, it might be more clear and robust to check that rather than checking for lexical lifetimes and debug users.

[atrick]

We currently build the standard library in a special mode that disables lexical lifetimes. It's important that we can eventually eliminate the special mode for the sanity of both standard library and compiler engineers.

https://github.com/apple/swift/blob/main/stdlib/cmake/modules/AddSwiftStdlib.cmake#L2146

A good test of whether this DestroyHoisting pass does not create performance problems will be if we can remove
-enable-lexical-lifetimes=false from the stdlib build.

My main concern is that, with this approach, we won't be able to optimize Array/String after inlining generic code.

[eeckstein]

@swift-ci smoke test

[atrick]

LGTM, but I am concerned about general long-term performance and specifically that this will make it harder to enable lexical lifetimes in the standard library because we won't be able to optimize array/string after specialization and inlining.

[atrick]

Answering my own previous question... in the future we could probably teach optimizations about @eagerMove types so we could still optimize Array/String lifetimes even without the lexical flag on an instruction.