swift icon indicating copy to clipboard operation
swift copied to clipboard

Fix ownership issues with sequences of partial_apply's in AutoDiff closure specialization pass

Open asl opened this issue 8 months ago • 9 comments

Each partial_apply consumes its arguments, therefore we should never release intermediate ones in the sequence of closures.

Fixes #78847

asl avatar Apr 09 '25 01:04 asl

I also added end-to-end swift test to test the pass, apparently there are no tests for this.

asl avatar Apr 09 '25 01:04 asl

@swift-ci please test

asl avatar Apr 09 '25 01:04 asl

The explanation of wrong behavior is in https://github.com/swiftlang/swift/issues/78847#issuecomment-2726038126

asl avatar Apr 09 '25 01:04 asl

Reenabling it back also gives huge performance boost. We are having (on benchmark from https://github.com/swiftlang/swift/pull/80665)

Before:

Benchmark_O-arm64-apple-macosx13.0 AutoDiffBuildingSimulator.Reverse
  # TEST                              SAMPLES      MIN   MEDIAN      MAX
 53 AutoDiffBuildingSimulator.Reverse         1   17.174   17.174   17.174

After:

Benchmark_O-arm64-apple-macosx13.0 AutoDiffBuildingSimulator.Reverse
  # TEST                              SAMPLES      MIN   MEDIAN      MAX
 53 AutoDiffBuildingSimulator.Reverse         1    4.670    4.670    4.670

asl avatar Apr 09 '25 02:04 asl

@swift-ci please test

asl avatar Apr 09 '25 02:04 asl

Tagging @JaapWijnen

asl avatar Apr 09 '25 02:04 asl

Nice! Also the performance boost. Is this boost related to the loss in performance we saw in January @asl ? Or separate and thus an improvement on top of the performance we had in early January?

JaapWijnen avatar Apr 09 '25 08:04 JaapWijnen

@JaapWijnen Correct. This brings the performance back to January figures

@eeckstein Here is what happened:

  • https://github.com/swiftlang/swift/issues/78847 disabled the autodiff closure specialization for SIL in OSSA form. But still it triggered earlier for code in non-OSSA form and required transformation happened (not sure why we did not see lifetime checker errors though...)
  • Later, the conversion to OSSA become much earlier. And therefore the pass always run on OSSA input. That together with https://github.com/swiftlang/swift/issues/78847 effectively disabled it
  • Turned out there were no e2e tests for the functionality (only unit tests for the pass itself), so this went unnoticed for quite some time.

asl avatar Apr 09 '25 18:04 asl

Looks good from what I can tell. Thanks for fixing!

jkshtj avatar Apr 09 '25 23:04 jkshtj

@swift-ci please test

asl avatar Apr 10 '25 22:04 asl

@swift-ci please smoke test

asl avatar Apr 15 '25 09:04 asl