Skip to content

[LVA] Update ignore instructions RLE and DSE. #31134

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

Merged
merged 1 commit into from
May 8, 2020

Conversation

zoecarver
Copy link
Contributor

@zoecarver zoecarver commented Apr 19, 2020

RLE can ignore end_access, set_deallocating, dealloc_ref, begin_access, and strong_release instructions.

DSE can ignore end_access, set_deallocating, begin_access, and sometimes strong_release.

Requires #31132

// If the strong release operand type can't have a custom destructor it's
// OK.
if (release->getOperand()->getType().getClassOrBoundGenericClass() ==
nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if the operand itself is not a class, it could be something (e.g. a thick function pointer) which contains a class (e.g. in the closure context).
So it's not safe to assume if it's not a class it cannot (indirectly) call a custom destructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I'll remove this check (ideally, I'd like to use something like dead object elimination's doesDestructorHaveSideEffects).

// We will look at all of its users and we can project to the source memory
// location so, if there are any actual writes to memory we will catch them
// there so, we can ignore begin_access here.
if (isa<BeginAccessInst>(Inst))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not add this to isRLEInertInstruction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're, right. That's probably a better place for it.

return;

// If the load is valid, then this strong release won't invoke the destructor.
// So we can ignore it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't give much context, sorry. I'll try to explain my reasoning a bit more clearly. The only case where we'd care about the strong_release is when it turns into a destructor with a read/write in it (in this case, only a write, actually). If this load is valid (this load being whatever load we happen to be promoting at any given time), then the strong_release can't have destroyed the objected we're loading before we load it. Does that make sense?

@zoecarver
Copy link
Contributor Author

I removed the access marker part of this patch. I'll add that back after the AccessPath patch lands. I'd really like to get this committed and it shouldn't be hard to review given the size and (new) scope.

@zoecarver
Copy link
Contributor Author

@swift-ci please test and benchmark

@zoecarver
Copy link
Contributor Author

@swift-ci test

@zoecarver
Copy link
Contributor Author

@swift-ci benchmark

@zoecarver
Copy link
Contributor Author

@swift-ci please benchmark

@swift-ci
Copy link
Contributor

swift-ci commented May 1, 2020

Performance: -O

Regression OLD NEW DELTA RATIO
Data.append.Sequence.64kB.Count.RE.I 21 44 +109.5% 0.48x
Data.append.Sequence.64kB.Count.RE 21 44 +109.5% 0.48x
PrefixAnySeqCRangeIter 13 26 +100.0% 0.50x
PrefixAnySeqCntRange 13 26 +100.0% 0.50x
DataAppendSequence 6500 9200 +41.5% 0.71x
Data.append.Sequence.809B.Count.RE 65 92 +41.5% 0.71x
Data.append.Sequence.809B.Count.RE.I 65 92 +41.5% 0.71x
EqualSubstringSubstring 22 30 +36.4% 0.73x
EqualSubstringSubstringGenericEquatable 22 30 +36.4% 0.73x
EqualSubstringString 22 30 +36.4% 0.73x
LessSubstringSubstring 23 30 +30.4% 0.77x (?)
EqualStringSubstring 23 30 +30.4% 0.77x
LessSubstringSubstringGenericComparable 23 30 +30.4% 0.77x
ObjectiveCBridgeStubDateAccess 130 152 +16.9% 0.86x
RemoveWhereMoveInts 19 22 +15.8% 0.86x
DataSubscriptSmall 13 15 +15.4% 0.87x
StringComparison_longSharedPrefix 322 358 +11.2% 0.90x (?)
AngryPhonebook 252 280 +11.1% 0.90x
RemoveWhereSwapInts 37 40 +8.1% 0.93x (?)
RGBHistogram 1460 1570 +7.5% 0.93x
 
Improvement OLD NEW DELTA RATIO
PrefixWhileAnySequence 1165 192 -83.5% 6.07x
PrefixAnySeqCRangeIterLazy 26 13 -50.0% 2.00x
PrefixAnySeqCntRangeLazy 26 13 -50.0% 2.00x
Set.subtracting.Seq.Int.Empty 183 110 -39.9% 1.66x
SetSymmetricDifferenceInt100 143 105 -26.6% 1.36x
DictionarySwap 868 660 -24.0% 1.32x
CharIteration_tweet_unicodeScalars 4840 3720 -23.1% 1.30x
CharIteration_ascii_unicodeScalars 2440 1880 -23.0% 1.30x
Set.isStrictSuperset.Seq.Box25 1108 874 -21.1% 1.27x (?)
DataCountSmall 19 15 -21.1% 1.27x
Set.isStrictSuperset.Seq.Box0 11060 8739 -21.0% 1.27x
DropWhileSequenceLazy 62 49 -21.0% 1.27x
Set.isStrictSubset.Seq.Box0 1108 876 -20.9% 1.26x
Set.isSubset.Seq.Box0 1109 877 -20.9% 1.26x
DictionarySwapAt 684 552 -19.3% 1.24x
Set.isStrictSubset.Seq.Box25 1226 991 -19.2% 1.24x (?)
Set.isSubset.Seq.Box25 1225 991 -19.1% 1.24x
DropWhileArrayLazy 58 49 -15.5% 1.18x
Set.subtracting.Seq.Empty.Int 129 110 -14.7% 1.17x
Set.intersection.Seq.Box0 320 273 -14.7% 1.17x
SetSymmetricDifferenceInt50 137 117 -14.6% 1.17x (?)
CharIteration_punctuated_unicodeScalars 560 480 -14.3% 1.17x
Set.intersection.Seq.Box25 412 354 -14.1% 1.16x (?)
DataCountMedium 17 15 -11.8% 1.13x
Prims.NonStrongRef.Unmanaged.Closure 173 153 -11.6% 1.13x (?)
Prims.NonStrongRef.Unmanaged 173 153 -11.6% 1.13x (?)
SetUnionBox25 264 234 -11.4% 1.13x (?)
WordCountUniqueASCII 1470 1310 -10.9% 1.12x (?)
SetSymmetricDifferenceBox25 400 357 -10.7% 1.12x (?)
Prims.NonStrongRef.UnmanagedUGR 173 155 -10.4% 1.12x (?)
Prims.NonStrongRef.UnmanagedUGR.Closure 174 156 -10.3% 1.12x (?)
Prims.NonStrongRef.UnownedUnsafe.Closure 218 196 -10.1% 1.11x
Prims.NonStrongRef.UnownedUnsafe 218 196 -10.1% 1.11x
SetUnion_OfObjects 4150 3740 -9.9% 1.11x (?)
SetUnionBox0 415 374 -9.9% 1.11x (?)
StringRemoveDupes 242 222 -8.3% 1.09x
WordCountUniqueUTF16 1610 1480 -8.1% 1.09x
SetSymmetricDifferenceInt25 127 117 -7.9% 1.09x (?)
SetExclusiveOr_OfObjects 5350 4930 -7.9% 1.09x (?)
SetSymmetricDifferenceBox0 535 493 -7.9% 1.09x (?)
CharIteration_punctuatedJapanese_unicodeScalars 560 520 -7.1% 1.08x

Code size: -O

Regression OLD NEW DELTA RATIO
PrimsNonStrongRef.o 121128 125048 +3.2% 0.97x
 
Improvement OLD NEW DELTA RATIO
PrefixWhile.o 18100 17558 -3.0% 1.03x
ExistentialPerformance.o 50428 49244 -2.3% 1.02x
DropWhile.o 18290 17940 -1.9% 1.02x
StringReplaceSubrange.o 5739 5659 -1.4% 1.01x
ObjectiveCBridgingStubs.o 16741 16517 -1.3% 1.01x
AngryPhonebook.o 10038 9926 -1.1% 1.01x

Performance: -Osize

Regression OLD NEW DELTA RATIO
EqualSubstringSubstring 23 31 +34.8% 0.74x
LessSubstringSubstring 23 31 +34.8% 0.74x
EqualStringSubstring 23 31 +34.8% 0.74x
EqualSubstringString 23 31 +34.8% 0.74x
LessSubstringSubstringGenericComparable 23 31 +34.8% 0.74x
CharIndexing_ascii_unicodeScalars 3560 4680 +31.5% 0.76x
EqualSubstringSubstringGenericEquatable 23 30 +30.4% 0.77x
CharIndexing_tweet_unicodeScalars 7200 9160 +27.2% 0.79x
CharIndexing_chinese_unicodeScalars 3520 4440 +26.1% 0.79x
CharIndexing_punctuated_unicodeScalars 920 1160 +26.1% 0.79x
CharIndexing_japanese_unicodeScalars 5920 7440 +25.7% 0.80x
CharIndexing_korean_unicodeScalars 4800 5880 +22.5% 0.82x
CharIndexing_russian_unicodeScalars 4160 5080 +22.1% 0.82x
CharIteration_japanese_unicodeScalars 3440 4200 +22.1% 0.82x
CharIndexing_punctuatedJapanese_unicodeScalars 960 1160 +20.8% 0.83x (?)
CharIndexing_utf16_unicodeScalars 5200 6280 +20.8% 0.83x
CharIteration_tweet_unicodeScalars 4280 5120 +19.6% 0.84x (?)
CharIteration_chinese_unicodeScalars 2040 2440 +19.6% 0.84x
CharIteration_korean_unicodeScalars 2600 3080 +18.5% 0.84x
CharIteration_ascii_unicodeScalars 2200 2600 +18.2% 0.85x
CharIteration_punctuatedJapanese_unicodeScalars 480 560 +16.7% 0.86x
CharIteration_punctuated_unicodeScalars 520 600 +15.4% 0.87x (?)
CharIteration_russian_unicodeScalars 2480 2840 +14.5% 0.87x
DropWhileAnySeqCntRange 98 111 +13.3% 0.88x
NopDeinit 8800 9900 +12.5% 0.89x
DropFirstAnyCollection 100 112 +12.0% 0.89x
AngryPhonebook 252 280 +11.1% 0.90x
DropWhileAnyCollection 109 121 +11.0% 0.90x
StringComparison_longSharedPrefix 321 356 +10.9% 0.90x (?)
SuffixAnyCollection 37 41 +10.8% 0.90x (?)
DropLastAnyCollection 37 41 +10.8% 0.90x (?)
CharIteration_utf16_unicodeScalars 3040 3280 +7.9% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
PrefixWhileAnySequence 1223 194 -84.1% 6.30x
Set.subtracting.Seq.Int.Empty 183 108 -41.0% 1.69x
DictionarySwap 860 664 -22.8% 1.30x
Set.isStrictSuperset.Seq.Box0 11299 8814 -22.0% 1.28x
Set.isStrictSuperset.Seq.Box25 1129 884 -21.7% 1.28x
Set.isSubset.Seq.Box0 1130 885 -21.7% 1.28x
Set.isStrictSubset.Seq.Box0 1130 886 -21.6% 1.28x (?)
DictionarySwapAt 684 548 -19.9% 1.25x
Set.isSubset.Seq.Box25 1245 1001 -19.6% 1.24x
Set.isStrictSubset.Seq.Box25 1244 1003 -19.4% 1.24x
Set.intersection.Seq.Box0 326 277 -15.0% 1.18x
Set.intersection.Seq.Box25 421 358 -15.0% 1.18x
CharIteration_russian_unicodeScalars_Backwards 4120 3520 -14.6% 1.17x
Set.subtracting.Seq.Empty.Int 129 112 -13.2% 1.15x (?)
SetUnionBox25 268 237 -11.6% 1.13x
Prims.NonStrongRef.UnmanagedUGR 184 163 -11.4% 1.13x (?)
SetSymmetricDifferenceBox25 405 359 -11.4% 1.13x (?)
Prims.NonStrongRef.UnmanagedUGR.Closure 184 165 -10.3% 1.12x (?)
Prims.NonStrongRef.Unmanaged.Closure 182 164 -9.9% 1.11x (?)
Prims.NonStrongRef.Unmanaged 182 164 -9.9% 1.11x (?)
SetUnion_OfObjects 4180 3770 -9.8% 1.11x (?)
SetUnionBox0 418 377 -9.8% 1.11x (?)
WordCountUniqueASCII 1470 1330 -9.5% 1.11x
PrefixWhileAnyCollection 153 139 -9.2% 1.10x
Prims.NonStrongRef.UnownedUnsafe.Closure 226 206 -8.8% 1.10x (?)
Prims.NonStrongRef.UnownedUnsafe 226 206 -8.8% 1.10x (?)
SortAdjacentIntPyramids 1140 1040 -8.8% 1.10x (?)
Walsh 630 579 -8.1% 1.09x (?)
SetExclusiveOr_OfObjects 5360 4930 -8.0% 1.09x (?)
SetSymmetricDifferenceBox0 536 493 -8.0% 1.09x
WordCountUniqueUTF16 1630 1510 -7.4% 1.08x
Set.subtracting.Seq.Box25 953 890 -6.6% 1.07x (?)

Code size: -Osize

Improvement OLD NEW DELTA RATIO
PrefixWhile.o 16684 16460 -1.3% 1.01x
DropWhile.o 18084 17856 -1.3% 1.01x

Performance: -Onone

Regression OLD NEW DELTA RATIO
EqualSubstringSubstring 27 34 +25.9% 0.79x
EqualSubstringSubstringGenericEquatable 27 34 +25.9% 0.79x
LessSubstringSubstringGenericComparable 27 34 +25.9% 0.79x
LessSubstringSubstring 28 35 +25.0% 0.80x
EqualStringSubstring 28 35 +25.0% 0.80x (?)
EqualSubstringString 28 35 +25.0% 0.80x
ObjectiveCBridgeStubNSDateRefAccess 4583 4998 +9.1% 0.92x (?)
 
Improvement OLD NEW DELTA RATIO
DictionarySwapAt 2736 2388 -12.7% 1.15x
SetSubtractingInt100 371 328 -11.6% 1.13x
DictionaryRemove 8450 7490 -11.4% 1.13x
ArrayOfPOD 723 645 -10.8% 1.12x (?)
SetSymmetricDifferenceInt100 518 471 -9.1% 1.10x (?)
Dict.CopyKeyValue.20k 3109 2829 -9.0% 1.10x (?)
Dict.CopyKeyValue.24k 3625 3313 -8.6% 1.09x (?)
Set.filter.Int100.20k 2460 2261 -8.1% 1.09x
Set.filter.Int100.24k 2889 2657 -8.0% 1.09x (?)
Dict.CopyKeyValue.28k 4776 4396 -8.0% 1.09x (?)
Dict.CopyKeyValue.16k 2584 2381 -7.9% 1.09x (?)
Set.filter.Int100.28k 3782 3495 -7.6% 1.08x (?)
Set.filter.Int100.16k 2070 1913 -7.6% 1.08x (?)
SetSubtractingInt50 306 283 -7.5% 1.08x (?)
Set.filter.Int50.16k 1247 1155 -7.4% 1.08x (?)
Set.filter.Int50.20k 1492 1382 -7.4% 1.08x (?)
Set.filter.Int50.24k 1756 1628 -7.3% 1.08x (?)
Set.filter.Int50.28k 2259 2096 -7.2% 1.08x (?)
DictionarySwap 2892 2684 -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 mini
  Model Identifier: Macmini8,1
  Processor Name: 6-Core Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

@swift-ci
Copy link
Contributor

swift-ci commented May 1, 2020

Build failed
Swift Test OS X Platform
Git Sha - 06932d0c87d3115302c966e18e78edfcf3a2a0c3

@zoecarver
Copy link
Contributor Author

I just ran the benchmarks on my machine and I got very different numbers:

------- Performance: -O -------

REGRESSION                                                  OLD     NEW     DELTA    RATIO
DataCountMedium                                             23      30      +30.4%   **0.77x**
CharIteration_punctuated_unicodeScalars                     640     720     +12.5%   **0.89x (?)**
CharIndexing_punctuatedJapanese_unicodeScalars_Backwards    1120    1240    +10.7%   **0.90x (?)**
CharIteration_korean_unicodeScalars                         3560    3920    +10.1%   **0.91x (?)**
CharIteration_chinese_unicodeScalars                        2840    3120    +9.9%    **0.91x (?)**
CharIteration_japanese_unicodeScalars                       4760    5200    +9.2%    **0.92x (?)**
NormalizedIterator_fastPrenormal                            860     930     +8.1%    **0.92x (?)**

IMPROVEMENT                                                 OLD     NEW     DELTA    RATIO
PrefixWhileAnySequence                                      1575    300     -81.0%   **5.25x**
Set.subtracting.Seq.Int.Empty                               253     151     -40.3%   **1.68x**
SetSymmetricDifferenceInt100                                207     148     -28.5%   **1.40x**
Set.isStrictSuperset.Seq.Box0                               14961   11540   -22.9%   **1.30x (?)**
Set.isStrictSuperset.Seq.Box25                              1488    1155    -22.4%   **1.29x (?)**
Set.isStrictSubset.Seq.Box0                                 1487    1158    -22.1%   **1.28x (?)**
Set.isSubset.Seq.Box0                                       1485    1158    -22.0%   **1.28x (?)**
DictionarySwap                                              1216    964     -20.7%   **1.26x (?)**
Set.isStrictSubset.Seq.Box25                                1648    1314    -20.3%   **1.25x (?)**
Set.isSubset.Seq.Box25                                      1647    1315    -20.2%   **1.25x (?)**
Set.subtracting.Seq.Empty.Int                               176     141     -19.9%   **1.25x (?)**
DictionarySwapAt                                            940     772     -17.9%   **1.22x (?)**
SetSymmetricDifferenceInt50                                 195     163     -16.4%   **1.20x (?)**
Set.intersection.Seq.Box0                                   431     366     -15.1%   **1.18x (?)**
RandomShuffleLCG2                                           768     656     -14.6%   **1.17x (?)**
Set.intersection.Seq.Box25                                  565     485     -14.2%   **1.16x (?)**
StringRemoveDupes                                           344     296     -14.0%   **1.16x (?)**
SetSymmetricDifferenceBox25                                 558     497     -10.9%   **1.12x (?)**
SetUnionBox25                                               372     332     -10.8%   **1.12x (?)**
Prims.NonStrongRef.Unmanaged                                235     210     -10.6%   **1.12x (?)**
StringToDataSmall                                           950     850     -10.5%   **1.12x (?)**
Prims.NonStrongRef.Unmanaged.Closure                        234     211     -9.8%    **1.11x (?)**
Prims.NonStrongRef.UnmanagedUGR.Closure                     235     212     -9.8%    **1.11x (?)**
CharIteration_punctuatedJapanese_unicodeScalars_Backwards   1240    1120    -9.7%    **1.11x (?)**
Prims.NonStrongRef.UnmanagedUGR                             235     215     -8.5%    **1.09x (?)**
Set.subtracting.Empty.Box                                   13      12      -7.7%    **1.08x (?)**
SetSymmetricDifferenceBox0                                  743     687     -7.5%    **1.08x (?)**
SetExclusiveOr_OfObjects                                    7430    6880    -7.4%    **1.08x (?)**
SetSymmetricDifferenceInt25                                 180     168     -6.7%    **1.07x (?)**
DataCreateEmpty                                             150     140     -6.7%    **1.07x (?)**
Set.subtracting.Seq.Box25                                   1322    1235    -6.6%    **1.07x (?)**

------- Performance: -Osize -------

REGRESSION                                OLD     NEW     DELTA    RATIO
DropLastAnyCollection                     45      49      +8.9%    **0.92x (?)**
Breadcrumbs.CopyUTF16CodeUnits.Mixed      82      89      +8.5%    **0.92x (?)**

IMPROVEMENT                               OLD     NEW     DELTA    RATIO
PrefixWhileAnySequence                    1749    285     -83.7%   **6.14x**
Set.subtracting.Seq.Int.Empty             253     154     -39.1%   **1.64x**
Set.subtracting.Seq.Empty.Int             176     135     -23.3%   **1.30x**
Set.isStrictSubset.Seq.Box0               1524    1184    -22.3%   **1.29x**
Set.isStrictSuperset.Seq.Box0             15246   11871   -22.1%   **1.28x**
Set.isSubset.Seq.Box0                     1522    1188    -21.9%   **1.28x (?)**
Set.isStrictSuperset.Seq.Box25            1520    1188    -21.8%   **1.28x (?)**
DictionarySwap                            1220    976     -20.0%   **1.25x (?)**
Set.isSubset.Seq.Box25                    1682    1347    -19.9%   **1.25x (?)**
Set.isStrictSubset.Seq.Box25              1682    1347    -19.9%   **1.25x (?)**
DictionarySwapAt                          948     768     -19.0%   **1.23x (?)**
Set.intersection.Seq.Box0                 441     372     -15.6%   **1.19x (?)**
CharIteration_ascii_unicodeScalars        2920    2480    -15.1%   **1.18x (?)**
StringRemoveDupes                         390     334     -14.4%   **1.17x (?)**
Set.intersection.Seq.Box25                576     495     -14.1%   **1.16x (?)**
WordCountUniqueASCII                      2050    1820    -11.2%   **1.13x (?)**
WordCountUniqueUTF16                      2350    2090    -11.1%   **1.12x (?)**
Prims.NonStrongRef.UnmanagedUGR.Closure   247     221     -10.5%   **1.12x (?)**

The regressions especially don't make sense to me. I looked at the SIL and IR for the Data.append tests and if anything they should be improvements, not regressions. Any idea why the results from the CI benchmark are so different?

@zoecarver zoecarver force-pushed the lva/ignore-in-dse branch from 06932d0 to b1d14cb Compare May 2, 2020 03:43
@zoecarver
Copy link
Contributor Author

I removed the strong_release change from the DSE part of this patch. The failing MacOS test exposed an edge case where set_deallocating doesn't mean the object is destroyed across all paths.

@zoecarver
Copy link
Contributor Author

@swift-ci please benchmark

@zoecarver
Copy link
Contributor Author

@swift-ci please test

@zoecarver
Copy link
Contributor Author

@swift-ci test

@zoecarver
Copy link
Contributor Author

@swift-ci benchmark

1 similar comment
@zoecarver
Copy link
Contributor Author

@swift-ci benchmark

@zoecarver
Copy link
Contributor Author

@swift-ci please test windows platform

@zoecarver
Copy link
Contributor Author

@swift-ci please test os x platform

@zoecarver
Copy link
Contributor Author

@swift-ci please benchmark

2 similar comments
@zoecarver
Copy link
Contributor Author

@swift-ci please benchmark

@zoecarver
Copy link
Contributor Author

@swift-ci please benchmark

@zoecarver
Copy link
Contributor Author

@swift-ci please test OS X platform

@zoecarver zoecarver requested a review from eeckstein May 5, 2020 19:06
@zoecarver
Copy link
Contributor Author

Ping. @atrick @eeckstein this patch should (now) only take a few minutes to review. And I'd really like to get it committed.

// being whatever load we happen to be promoting at any given time), then the
// strong_release can't have destroyed the objected we're loading before we
// load it. So we can safely ignore the strong_release.
case SILInstructionKind::StrongReleaseInst:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that including strong_release is safe here. A deinit can have any side effects, e.g. writing to global variables, etc. What about (in pseudo SIL)

  load %address_of_global_variable
  strong_release %unrelated_object
  load %address_of_global_variable

and the deinit of the unrelated object contains

  store %whatever to %address_of_global_variable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, you're right. I didn't think of that. I'll remove the strong_release from this patch and make another one that does more deinit analysis.

RLE can ignore end_access, set_deallocating, and dealloc_ref instructions.

DSE can ignore end_access and set_deallocating.
@zoecarver zoecarver force-pushed the lva/ignore-in-dse branch from b1d14cb to 34ad7d0 Compare May 7, 2020 15:49
@zoecarver zoecarver requested a review from eeckstein May 7, 2020 15:51
Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@zoecarver
Copy link
Contributor Author

@eeckstein fantastic. Thank you for reviewing :)

@zoecarver
Copy link
Contributor Author

@swift-ci please benchmark

@zoecarver
Copy link
Contributor Author

@swift-ci please test

@zoecarver
Copy link
Contributor Author

@swift-ci test and merge

@swift-ci
Copy link
Contributor

swift-ci commented May 7, 2020

Build failed
Swift Test Linux Platform
Git Sha - b1d14cbc18fb862c04d4a90743d1bcb3505790d6

@zoecarver
Copy link
Contributor Author

@swift-ci benchmark

@eeckstein
Copy link
Contributor

I'll remove the strong_release from this patch and make another one that does more deinit analysis.

Note that the compiler already does some analysis of release instructions. See MemoryBehaviorVisitor::visitStrongReleaseInst (which is used by AliasAnalysis, which is used by RLE and DSE).
If you want to work on this, I recommend you experiment a bit and if something can be improved, add it to MemoryBehaviorVisitor::visitStrongReleaseInst.

@zoecarver
Copy link
Contributor Author

@eeckstein good to know. I'll take a look and play around with that. DeadObjectElimination::doesDestructorHaveSideEffects does the analysis we need here so, I was planning to generalize that and use it in DSE and RLE.

@swift-ci
Copy link
Contributor

swift-ci commented May 7, 2020

Build failed
Swift Test OS X Platform
Git Sha - 34ad7d0

@zoecarver
Copy link
Contributor Author

Looks like an issue with the bot:

/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/include/swift/ABI/Metadata.h:44:10: fatal error: cannot open file '/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.4.sdk/usr/include/objc/runtime.h': Operation not permitted
11:55:41 #include <objc/runtime.h>
11:55:41          ^
11:55:41 1 error generated.

Looking at the history, it seems like the last few pull request tests failed for the same reason (on that bot).

@zoecarver
Copy link
Contributor Author

@swift-ci test OS X platform

@swift-ci
Copy link
Contributor

swift-ci commented May 7, 2020

Performance: -O

Improvement OLD NEW DELTA RATIO
FlattenListFlatMap 8522 6441 -24.4% 1.32x (?)

Code size: -O

Improvement OLD NEW DELTA RATIO
PrefixWhile.o 18100 17734 -2.0% 1.02x
DropWhile.o 18290 17956 -1.8% 1.02x

Performance: -Osize

Regression OLD NEW DELTA RATIO
ObjectiveCBridgeStubToNSDate2 500 560 +12.0% 0.89x (?)

Code size: -Osize

Performance: -Onone

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: 8-Core Intel Xeon E5
  Processor Speed: 3 GHz
  Number of Processors: 1
  Total Number of Cores: 8
  L2 Cache (per Core): 256 KB
  L3 Cache: 25 MB
  Memory: 64 GB

@zoecarver
Copy link
Contributor Author

zoecarver commented May 7, 2020

Looks like most of the performance was in the strong_release part of this patch 😕It should still unlock some performance in #31423, though.

@zoecarver
Copy link
Contributor Author

@swift-ci test linux platform

1 similar comment
@zoecarver
Copy link
Contributor Author

@swift-ci test linux platform

@swift-ci
Copy link
Contributor

swift-ci commented May 8, 2020

Build failed
Swift Test Linux Platform
Git Sha - 34ad7d0

@zoecarver
Copy link
Contributor Author

That again looks like an unrelated issue. But, that's the third time in a row I've gotten some kind of flakiness from the linux tests. Is there some underlying issue or is it just bad luck?

@zoecarver
Copy link
Contributor Author

@swift-ci please test linux platform

@zoecarver
Copy link
Contributor Author

Looks like it was just bad luck :P

@zoecarver zoecarver merged commit 3d532cb into swiftlang:master May 8, 2020
@@ -160,6 +160,9 @@ static bool isRLEInertInstruction(SILInstruction *Inst) {
case SILInstructionKind::IsEscapingClosureInst:
case SILInstructionKind::IsUniqueInst:
case SILInstructionKind::FixLifetimeInst:
case SILInstructionKind::EndAccessInst:
case SILInstructionKind::SetDeallocatingInst:
case SILInstructionKind::DeallocRefInst:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can centralize this notion in a helper. Just a random thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot that could be centralized between DSE and RLE. They both end up doing the work of the other pass anyway so, it might make sense to combine them.

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