Skip to content

[ownership] Move lowering past SILCombine and right before the inliner. #35802

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Feb 6, 2021

Just a good place to test this functionality separately from the inliner.

NOTE: There is currently a draft since there is one commit I am going to get in via a different PR. This PR I am going to use initially for testing and maybe once that other commit lands, rebase on top and just move the passes as an independent change into the tree.

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

@swift-ci test

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

@swift-ci test source compatibility

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

@swift-ci benchmark

…unchecked_take_enum_data_addr + load -> load + unchecked_enum_data.

We must make sure the new load is inserted where the old load was instead of
inserting it at the unchecked_take_enum_data_addr, since the
unchecked_take_enum_data_addr may be in a different block from old load /and/
old load may not post-dominate that point. This was just a thinko.
… run.

I am doing this to separate the perf effect of moving ownership past the inliner
with this earlier stuff.
@gottesmm gottesmm force-pushed the move-ownership-past-sil-combine-before-inliner branch from da69a5f to 5ab8022 Compare February 6, 2021 02:02
@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

@swift-ci test

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

@swift-ci benchmark

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

@swift-ci test source compatibility

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

@swift-ci smoke test compiler performance

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

Just to note, current source compat release failures:

18:12:47   FAIL: Dollar, 5.1, 4feadc, Dollar, generic/platform=macOS
18:12:47   FAIL: Dollar, 4.0, 433d4b, Dollar, generic/platform=macOS
18:12:47   FAIL: GRDB.swift, 5.0, 759c10, Swift Package
18:12:47   FAIL: GRDB.swift, 4.2, 759c10, Swift Package
18:12:47   FAIL: Kitura, 5.1, 8578c4, Swift Package
18:12:47   FAIL: Kitura, 4.0, f1c316, Swift Package
18:12:47   FAIL: ProcedureKit, 5.1, a7fa56, ProcedureKitCloud, platform=macOS
18:12:47   FAIL: ProcedureKit, 5.1, a7fa56, ProcedureKit, generic/platform=tvOS
18:12:47   FAIL: ProcedureKit, 5.1, a7fa56, ProcedureKit, generic/platform=iOS
18:12:47   FAIL: ProcedureKit, 5.1, a7fa56, ProcedureKit, platform=macOS
18:12:47   FAIL: ProcedureKit, 5.0, a7fa56, ProcedureKitCloud, platform=macOS
18:12:47   FAIL: ProcedureKit, 5.0, a7fa56, ProcedureKit, generic/platform=tvOS
18:12:47   FAIL: ProcedureKit, 5.0, a7fa56, ProcedureKit, generic/platform=iOS
18:12:47   FAIL: ProcedureKit, 5.0, a7fa56, ProcedureKit, platform=macOS
18:12:47   FAIL: ProcedureKit, 4.2, 94193d, ProcedureKitCloud, platform=macOS
18:12:47   FAIL: ProcedureKit, 4.2, 94193d, ProcedureKit, generic/platform=tvOS
18:12:47   FAIL: ProcedureKit, 4.2, 94193d, ProcedureKit, generic/platform=iOS
18:12:47   FAIL: ProcedureKit, 4.2, 94193d, ProcedureKit, platform=macOS
18:12:47   FAIL: swift-log, 5.2, 16cfcc, Swift Package
18:12:47   FAIL: swift-log, 5.0, 16cfcc, Swift Package
18:12:47   FAIL: async-http-client, 5.0, a72c5a, Swift Package
18:12:47   FAIL: swift-distributed-tracing, 5.2, 73f2b5, Swift Package
18:12:47   FAIL: swift-distributed-tracing, 5.0, 73f2b5, Swift Package
18:12:47   FAIL: swift-cluster-membership, 5.2, 0fd434, Swift Package
18:12:47   FAIL: SwiftyJSON, 5.1, bad5f2, SwiftyJSON watchOS, generic/platform=watchOS
18:12:47   FAIL: SwiftyJSON, 5.1, bad5f2, SwiftyJSON tvOS, generic/platform=tvOS
18:12:47   FAIL: SwiftyJSON, 5.1, bad5f2, SwiftyJSON iOS, generic/platform=iOS
18:12:47   FAIL: SwiftyJSON, 5.1, bad5f2, SwiftyJSON macOS, platform=macOS
18:12:47   FAIL: SwiftyJSON, 5.0, b93c5d, SwiftyJSON watchOS, generic/platform=watchOS
18:12:47   FAIL: SwiftyJSON, 5.0, b93c5d, SwiftyJSON tvOS, generic/platform=tvOS
18:12:47   FAIL: SwiftyJSON, 5.0, b93c5d, SwiftyJSON iOS, generic/platform=iOS
18:12:47   FAIL: SwiftyJSON, 5.0, b93c5d, SwiftyJSON macOS, platform=macOS
18:12:47   FAIL: SwiftyJSON, 4.2, b93c5d, SwiftyJSON watchOS, generic/platform=watchOS
18:12:47   FAIL: SwiftyJSON, 4.2, b93c5d, SwiftyJSON tvOS, generic/platform=tvOS
18:12:47   FAIL: SwiftyJSON, 4.2, b93c5d, SwiftyJSON iOS, generic/platform=iOS
18:12:47   FAIL: SwiftyJSON, 4.2, b93c5d, SwiftyJSON macOS, platform=macOS
18:12:47   FAIL: vapor_console, 5.1, fcd8c1, Swift Package
18:12:47 ========================================

And the debug has had issues over the past bit just building. Just going to make sure I didn't make this worse.

@swift-ci
Copy link
Contributor

swift-ci commented Feb 6, 2021

Performance: -O

Regression OLD NEW DELTA RATIO
FlattenListLoop 1682 2515 +49.5% 0.67x (?)
StringInterpolation 10800 12400 +14.8% 0.87x (?)
ObjectiveCBridgeFromNSArrayAnyObjectForced 4500 4960 +10.2% 0.91x (?)
StringBuilderLong 1350 1480 +9.6% 0.91x (?)
 
Improvement OLD NEW DELTA RATIO
StringFromLongWholeSubstringGeneric 8 5 -37.5% 1.60x
LessSubstringSubstring 44 39 -11.4% 1.13x
StringToDataMedium 3800 3400 -10.5% 1.12x (?)
EqualSubstringSubstring 42 39 -7.1% 1.08x (?)
EqualSubstringString 42 39 -7.1% 1.08x (?)
EqualStringSubstring 43 40 -7.0% 1.07x
EqualSubstringSubstringGenericEquatable 43 40 -7.0% 1.07x
LessSubstringSubstringGenericComparable 43 40 -7.0% 1.07x (?)
SortStringsUnicode 3100 2895 -6.6% 1.07x (?)

Code size: -O

Performance: -Osize

Regression OLD NEW DELTA RATIO
Diffing.Same 8 9 +12.5% 0.89x (?)
ObjectiveCBridgeFromNSArrayAnyObjectForced 4660 5120 +9.9% 0.91x (?)
 
Improvement OLD NEW DELTA RATIO
StringFromLongWholeSubstringGeneric 8 5 -37.5% 1.60x
String.data.LargeUnicode 126 106 -15.9% 1.19x (?)
FlattenListFlatMap 7092 6061 -14.5% 1.17x (?)
LessSubstringSubstringGenericComparable 44 39 -11.4% 1.13x
LessSubstringSubstring 43 39 -9.3% 1.10x (?)
InsertCharacterEndIndexNonASCII 65 59 -9.2% 1.10x (?)
EqualStringSubstring 44 40 -9.1% 1.10x (?)
EqualSubstringSubstringGenericEquatable 44 40 -9.1% 1.10x (?)
StringWalk 2720 2520 -7.4% 1.08x
StrComplexWalk 5150 4780 -7.2% 1.08x

Code size: -Osize

Performance: -Onone

Regression OLD NEW DELTA RATIO
FloatingPointPrinting_Float_description_uniform 19800 25100 +26.8% 0.79x (?)
FloatingPointPrinting_Float_description_small 22140 27216 +22.9% 0.81x (?)
StringBuilderLong 2350 2720 +15.7% 0.86x (?)
ArrayLiteral2 3286 3743 +13.9% 0.88x (?)
PrefixWhileCountableRangeLazy 52589 56776 +8.0% 0.93x (?)
ArrayValueProp 5196 5606 +7.9% 0.93x (?)
ArrayOfPOD 1061 1142 +7.6% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
StringFromLongWholeSubstringGeneric 99 90 -9.1% 1.10x (?)
DataReplaceSmallBuffer 6000 5500 -8.3% 1.09x (?)
ClassArrayGetter2 4830 4490 -7.0% 1.08x (?)

Code size: -swiftlibs

How to read the data The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview
  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

@swift-ci benchmark

@gottesmm gottesmm changed the title [DRAFT][ownership] Move lowering past SILCombine and right before the inliner. [ownership] Move lowering past SILCombine and right before the inliner. Feb 6, 2021
@gottesmm gottesmm marked this pull request as ready for review February 6, 2021 06:12
@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

Release source compat failed due to a weird module map issue.

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

Not this PR for sure.

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

Debug one hit the same thing:

23:19:17 49%: Compile Triple.cpp (x86_64)
23:19:17 49%: Compile Swift source files (arm64)
23:19:17 fatal error: module map file '/Users/buildnode/jenkins/workspace-private/swift-PR-source-compat-suite-debug/build/compat_macos/swiftpm-macosx-x86_64/apple/Intermediates.noindex/GeneratedModuleMaps/macosx/CCryptoBoringSSL.modulemap' not found
23:19:17 
23:19:17 fatal error: module map file '/Users/buildnode/jenkins/workspace-private/swift-PR-source-compat-suite-debug/build/compat_macos/swiftpm-macosx-x86_64/apple/Intermediates.noindex/GeneratedModuleMaps/macosx/CCryptoBoringSSL.modulemap' not found
23:19:17 
23:19:17 fatal error: module map file '/Users/buildnode/jenkins/workspace-private/swift-PR-source-compat-suite-debug/build/compat_macos/swiftpm-macosx-x86_64/apple/Intermediates.noindex/GeneratedModuleMaps/macosx/CCryptoBoringSSL.modulemap' not found
23:19:17 
23:19:17 fatal error: module map file '/Users/buildnode/jenkins/workspace-private/swift-PR-source-compat-suite-debug/build/compat_macos/swiftpm-macosx-x86_64/apple/Intermediates.noindex/GeneratedModuleMaps/macosx/CCryptoBoringSSL.modulemap' not found
23:19:17 
23:19:18 49%: Compile Threading.cpp (x86_64)
23:19:18 49%: Compile TargetParser.cpp (x86_64)
23:19:18 1 error generated.
23:19:18 
23:19:18 2 errors generated.
23:19:18 
23:19:18 1 error generated.
23:19:18 
23:19:18 Build cancelled
23:19:18 env SDKROOT=/Applicat

@swift-ci
Copy link
Contributor

swift-ci commented Feb 6, 2021

Performance: -O

Regression OLD NEW DELTA RATIO
UTF8Decode_InitFromData_ascii_as_ascii 628 742 +18.2% 0.85x (?)
StringToDataLargeUnicode 3450 3900 +13.0% 0.88x (?)
StringInterpolation 10900 12300 +12.8% 0.89x (?)
UTF8Decode_InitFromBytes_ascii_as_ascii 445 497 +11.7% 0.90x (?)
ObjectiveCBridgeToNSDictionary 15000 16200 +8.0% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
StringFromLongWholeSubstringGeneric 8 5 -37.5% 1.60x
FlattenListFlatMap 4547 3889 -14.5% 1.17x (?)
LessSubstringSubstring 44 39 -11.4% 1.13x (?)
DictionaryKeysContainsCocoa 35 32 -8.6% 1.09x (?)
ObjectiveCBridgeStubFromArrayOfNSString2 2630 2440 -7.2% 1.08x (?)
EqualSubstringString 42 39 -7.1% 1.08x (?)
EqualStringSubstring 43 40 -7.0% 1.07x (?)

Code size: -O

Performance: -Osize

Improvement OLD NEW DELTA RATIO
StringFromLongWholeSubstringGeneric 8 5 -37.5% 1.60x
EqualStringSubstring 44 39 -11.4% 1.13x (?)
LessSubstringSubstringGenericComparable 44 39 -11.4% 1.13x
LessSubstringSubstring 43 39 -9.3% 1.10x
InsertCharacterEndIndexNonASCII 65 59 -9.2% 1.10x (?)
EqualSubstringSubstringGenericEquatable 44 40 -9.1% 1.10x (?)
StringWalk 2720 2520 -7.4% 1.08x (?)
EqualSubstringSubstring 42 39 -7.1% 1.08x (?)
EqualSubstringString 42 39 -7.1% 1.08x (?)
StrComplexWalk 5150 4790 -7.0% 1.08x (?)
InsertCharacterEndIndex 177 165 -6.8% 1.07x (?)

Code size: -Osize

Performance: -Onone

Regression OLD NEW DELTA RATIO
FloatingPointPrinting_Float_description_uniform 19800 25000 +26.3% 0.79x (?)
FloatingPointPrinting_Float_description_small 22140 27216 +22.9% 0.81x
UTF8Decode_InitFromBytes_ascii_as_ascii 468 523 +11.8% 0.89x (?)
StringToDataLargeUnicode 8650 9450 +9.2% 0.92x (?)
ArrayOfPOD 1060 1142 +7.7% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
StringFromLongWholeSubstringGeneric 99 90 -9.1% 1.10x (?)
ClassArrayGetter2 4830 4490 -7.0% 1.08x (?)

Code size: -swiftlibs

How to read the data The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview
  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

@swift-ci
Copy link
Contributor

swift-ci commented Feb 6, 2021

Compilation-performance test failed

@LucianoPAlmeida
Copy link
Contributor

Release source compat failed due to a weird module map issue.

@gottesmm There is a discussion about that on #35503 :)

@tbkka
Copy link
Contributor

tbkka commented Feb 6, 2021

This one I happen to know something about: FloatingPointPrinting_Float_description_uniform. That test just converts a bunch of float values to string in a tight loop. Interesting point: those strings should all be small, so it should not be creating refcounted objects at all.

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

I don't think the perf changes are real. Going to run again.

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

@swift-ci benchmark

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

Not real in the sense that the regressions are changing each time suggesting noise.

@swift-ci
Copy link
Contributor

swift-ci commented Feb 6, 2021

Performance: -O

Improvement OLD NEW DELTA RATIO
StringFromLongWholeSubstringGeneric 7 5 -28.6% 1.40x
UTF8Decode_InitFromData_ascii_as_ascii 748 659 -11.9% 1.14x (?)
String.data.LargeUnicode 111 101 -9.0% 1.10x (?)

Code size: -O

Performance: -Osize

Regression OLD NEW DELTA RATIO
FlattenListFlatMap 3888 6093 +56.7% 0.64x (?)
UTF8Decode_InitFromData_ascii_as_ascii 612 662 +8.2% 0.92x (?)
 
Improvement OLD NEW DELTA RATIO
StringFromLongWholeSubstringGeneric 7 5 -28.6% 1.40x
InsertCharacterEndIndexNonASCII 65 59 -9.2% 1.10x (?)
InsertCharacterEndIndex 181 165 -8.8% 1.10x (?)

Code size: -Osize

Performance: -Onone

Regression OLD NEW DELTA RATIO
FloatingPointPrinting_Float_description_uniform 19700 25100 +27.4% 0.78x (?)
FloatingPointPrinting_Float_description_small 22032 27108 +23.0% 0.81x
StringBuilderLong 2280 2610 +14.5% 0.87x (?)
ErrorHandling 4590 5240 +14.2% 0.88x (?)
StringBuilderSmallReservingCapacity 3012 3264 +8.4% 0.92x (?)
DropWhileAnySeqCntRange 69710 75477 +8.3% 0.92x (?)
StringUTF16Builder 3080 3330 +8.1% 0.92x (?)
StringBuilder 3044 3279 +7.7% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
StringFromLongWholeSubstringGeneric 99 90 -9.1% 1.10x (?)
ClassArrayGetter2 4840 4490 -7.2% 1.08x (?)

Code size: -swiftlibs

How to read the data The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview
  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

If you look at the 3 groups of results for -O, -Osize every time the regressions are changing suggesting they are noise. Consider for -O:

FlattenListLoop | 1682 | 2515 | +49.5% | 0.67x (?)
StringInterpolation | 10800 | 12400 | +14.8% | 0.87x (?)
ObjectiveCBridgeFromNSArrayAnyObjectForced | 4500 | 4960 | +10.2% | 0.91x (?)
StringBuilderLong | 1350 | 1480 | +9.6% | 0.91x (?)
UTF8Decode_InitFromData_ascii_as_ascii | 628 | 742 | +18.2% | 0.85x (?)
StringToDataLargeUnicode | 3450 | 3900 | +13.0% | 0.88x (?)
StringInterpolation | 10900 | 12300 | +12.8% | 0.89x (?)
UTF8Decode_InitFromBytes_ascii_as_ascii | 445 | 497 | +11.7% | 0.90x (?)
ObjectiveCBridgeToNSDictionary | 15000 | 16200 | +8.0% | 0.93x (?)
StringFromLongWholeSubstringGeneric | 7 | 5 | -28.6% | 1.40x
UTF8Decode_InitFromData_ascii_as_ascii | 748 | 659 | -11.9% | 1.14x (?)
String.data.LargeUnicode | 111 | 101 | -9.0% | 1.10x (?)

And for -Osize:

Diffing.Same | 8 | 9 | +12.5% | 0.89x (?)
ObjectiveCBridgeFromNSArrayAnyObjectForced | 4660 | 5120 | +9.9% | 0.91x (?)
StringFromLongWholeSubstringGeneric | 8 | 5 | -37.5% | 1.60x
EqualStringSubstring | 44 | 39 | -11.4% | 1.13x (?)
LessSubstringSubstringGenericComparable | 44 | 39 | -11.4% | 1.13x
LessSubstringSubstring | 43 | 39 | -9.3% | 1.10x
InsertCharacterEndIndexNonASCII | 65 | 59 | -9.2% | 1.10x (?)
EqualSubstringSubstringGenericEquatable | 44 | 40 | -9.1% | 1.10x (?)
StringWalk | 2720 | 2520 | -7.4% | 1.08x (?)
EqualSubstringSubstring | 42 | 39 | -7.1% | 1.08x (?)
EqualSubstringString | 42 | 39 | -7.1% | 1.08x (?)
StrComplexWalk | 5150 | 4790 | -7.0% | 1.08x (?)
InsertCharacterEndIndex | 177 | 165 | -6.8% | 1.07x (?)
FlattenListFlatMap | 3888 | 6093 | +56.7% | 0.64x (?)
UTF8Decode_InitFromData_ascii_as_ascii | 612 | 662 | +8.2% | 0.92x (?)

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 6, 2021

Merging!

@gottesmm gottesmm merged commit fae2564 into swiftlang:main Feb 6, 2021
@gottesmm gottesmm deleted the move-ownership-past-sil-combine-before-inliner branch February 6, 2021 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants