Skip to content

Avoid introducing critical edges in SimplifyCFG #34362

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Avoid introducing critical edges in SimplifyCFG #34362

wants to merge 3 commits into from

Conversation

atrick
Copy link
Contributor

@atrick atrick commented Oct 20, 2020

Passes will end up resplitting them, forcing SimplifyCFG to
continually rerun.

Also, we want to allow SIL utilities to assume no critical edges, and
avoid the need for several passes to internally split edges and modify
the CFG for no reason.

Preparation for adding invariants that simplify working with OSSA form and opaque values.

@atrick atrick requested a review from eeckstein October 20, 2020 07:28
@atrick
Copy link
Contributor Author

atrick commented Oct 20, 2020

@swift-ci test

@atrick
Copy link
Contributor Author

atrick commented Oct 20, 2020

@swift-ci benchmark

@swift-ci
Copy link
Contributor

Performance: -O

Regression OLD NEW DELTA RATIO
SuffixSequence 171 1811 +959.1% 0.09x
SuffixSequenceLazy 171 1774 +937.4% 0.10x
FlattenListFlatMap 3383 18791 +455.5% 0.18x
SortIntPyramid 455 740 +62.6% 0.61x
SortAdjacentIntPyramids 640 945 +47.7% 0.68x
ArrayAppendSequence 500 670 +34.0% 0.75x
AngryPhonebook.ASCII2 109 142 +30.3% 0.77x (?)
ObjectiveCBridgeStubDataAppend 1820 2300 +26.4% 0.79x
DropWhileAnySeqCRangeIterLazy 55 69 +25.5% 0.80x
DropWhileAnySeqCntRangeLazy 55 69 +25.5% 0.80x
Data.init.Sequence.64kB.Count0.I 140 171 +22.1% 0.82x
RemoveWhereSwapInts 10 12 +20.0% 0.83x (?)
Data.init.Sequence.64kB.Count0 142 169 +19.0% 0.84x
DropWhileArrayLazy 49 58 +18.4% 0.84x
UTF8Decode_InitDecoding 144 170 +18.1% 0.85x
Data.append.Sequence.64kB.Count0 140 165 +17.9% 0.85x (?)
Data.append.Sequence.64kB.Count0.I 140 165 +17.9% 0.85x (?)
Data.append.Sequence.809B.Count0 205 240 +17.1% 0.85x (?)
ObjectiveCBridgeStubDateAccess 130 152 +16.9% 0.86x
Set.isDisjoint.Seq.Empty.Int 79 92 +16.5% 0.86x (?)
Data.append.Sequence.809B.Count0.I 212 246 +16.0% 0.86x (?)
UTF8Decode_InitFromCustom_contiguous 145 168 +15.9% 0.86x
DistinctClassFieldAccesses 183 212 +15.8% 0.86x (?)
Data.init.Sequence.809B.Count0 247 284 +15.0% 0.87x (?)
SuffixAnySequence 1631 1864 +14.3% 0.88x
Data.append.Sequence.64kB.Count0.RE 146 166 +13.7% 0.88x (?)
IterateData 871 990 +13.7% 0.88x (?)
Data.append.Sequence.64kB.Count0.RE.I 146 165 +13.0% 0.88x (?)
Data.init.Sequence.809B.Count0.I 247 279 +13.0% 0.89x (?)
Data.append.Sequence.809B.Count0.RE 219 245 +11.9% 0.89x
Set.isDisjoint.Seq.Int100 115 128 +11.3% 0.90x
NormalizedIterator_ascii 80 89 +11.2% 0.90x (?)
Set.isDisjoint.Seq.Int.Empty 46 51 +10.9% 0.90x (?)
DataSubscriptMedium 37 41 +10.8% 0.90x (?)
Data.init.Sequence.2049B.Count0.I 286 316 +10.5% 0.91x (?)
DictionarySwap 612 676 +10.5% 0.91x (?)
Data.init.Sequence.2047B.Count0.I 285 312 +9.5% 0.91x (?)
StringToDataSmall 550 600 +9.1% 0.92x (?)
Data.init.Sequence.511B.Count0.I 279 304 +9.0% 0.92x (?)
UTF8Decode_InitFromCustom_noncontiguous 274 298 +8.8% 0.92x (?)
Set.isSubset.Seq.Empty.Int 80 87 +8.7% 0.92x (?)
Set.isDisjoint.Empty.Int 86 93 +8.1% 0.92x (?)
Set.isDisjoint.Empty.Box 87 94 +8.0% 0.93x (?)
Set.isSuperset.Seq.Int.Empty 87 94 +8.0% 0.93x (?)
Set.isDisjoint.Seq.Empty.Box 87 94 +8.0% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
DropLastAnySequence 1483 347 -76.6% 4.27x
Data.append.Sequence.64kB.Count.RE.I 44 20 -54.5% 2.20x
Data.append.Sequence.64kB.Count.RE 44 20 -54.5% 2.20x
ArrayAppendLazyMap 1560 850 -45.5% 1.84x
RangeOverlapsClosedRange 115 74 -35.7% 1.55x
Data.append.Sequence.809B.Count.RE.I 91 59 -35.2% 1.54x
DataAppendSequence 8900 5900 -33.7% 1.51x
Data.append.Sequence.809B.Count.RE 89 60 -32.6% 1.48x
RemoveWhereSwapStrings 432 308 -28.7% 1.40x
SetSymmetricDifferenceInt100 145 107 -26.2% 1.36x
ArrayAppendLatin1Substring 25488 19296 -24.3% 1.32x
ArrayAppendUTF16Substring 24912 18900 -24.1% 1.32x
ArrayAppendAsciiSubstring 25452 19332 -24.0% 1.32x
SetSymmetricDifferenceInt50 140 118 -15.7% 1.19x (?)
ReversedBidirectional 11581 9990 -13.7% 1.16x (?)
CharIndexing_punctuatedJapanese_unicodeScalars 880 760 -13.6% 1.16x
CharIndexing_punctuated_unicodeScalars 920 800 -13.0% 1.15x
DataCountMedium 17 15 -11.8% 1.13x (?)
CharIndexing_utf16_unicodeScalars 5080 4520 -11.0% 1.12x (?)
SetSymmetricDifferenceInt25 133 120 -9.8% 1.11x (?)
Calculator 158 143 -9.5% 1.10x (?)
DataSetCountMedium 220 200 -9.1% 1.10x (?)
AngryPhonebook.Strasse 140 128 -8.6% 1.09x (?)
AngryPhonebook.Armenian 155 143 -7.7% 1.08x (?)
AngryPhonebook.Cyrillic 167 155 -7.2% 1.08x (?)

Code size: -O

Regression OLD NEW DELTA RATIO
Suffix.o 20925 26085 +24.7% 0.80x
PrimsNonStrongRef.o 124508 131500 +5.6% 0.95x
StringRemoveDupes.o 5373 5485 +2.1% 0.98x
DictTest3.o 14973 15181 +1.4% 0.99x
UTF8Decode.o 23539 23827 +1.2% 0.99x
DictTest.o 13828 13972 +1.0% 0.99x
 
Improvement OLD NEW DELTA RATIO
main.o 57911 49415 -14.7% 1.17x
ObjectiveCBridgingStubs.o 16085 14101 -12.3% 1.14x
Mirror.o 10854 10086 -7.1% 1.08x
FlattenList.o 3914 3690 -5.7% 1.06x
RangeOverlaps.o 6038 5798 -4.0% 1.04x
DictTest4.o 15674 15226 -2.9% 1.03x
DropLast.o 20639 20142 -2.4% 1.02x
SortLettersInPlace.o 8038 7873 -2.1% 1.02x
ArrayAppend.o 22967 22543 -1.8% 1.02x
COWTree.o 10604 10428 -1.7% 1.02x
Prefix.o 15080 14888 -1.3% 1.01x
CSVParsing.o 53379 52723 -1.2% 1.01x
DropFirst.o 16932 16740 -1.1% 1.01x

Performance: -Osize

Regression OLD NEW DELTA RATIO
SuffixSequenceLazy 623 2089 +235.3% 0.30x
SuffixSequence 624 1971 +215.9% 0.32x
Data.init.Sequence.64kB.Count 29 58 +100.0% 0.50x
Data.init.Sequence.64kB.Count.I 30 59 +96.7% 0.51x
Data.init.Sequence.2047B.Count.I 54 101 +87.0% 0.53x
Data.init.Sequence.2049B.Count.I 54 101 +87.0% 0.53x
DropLastArrayLazy 5 9 +80.0% 0.56x
SuffixArray 5 9 +80.0% 0.56x
Data.init.Sequence.809B.Count.I 53 91 +71.7% 0.58x
Data.init.Sequence.809B.Count 54 91 +68.5% 0.59x
Set.isSuperset.Seq.Box25 119 194 +63.0% 0.61x
Data.init.Sequence.511B.Count.I 68 103 +51.5% 0.66x
Data.init.Sequence.513B.Count.I 67 101 +50.7% 0.66x
ParseInt.UInt64.Decimal 178 267 +50.0% 0.67x
Data.init.Sequence.64kB.Count.RE.I 20 30 +50.0% 0.67x
Data.init.Sequence.64kB.Count.RE 20 30 +50.0% 0.67x
RemoveWhereMoveInts 6 9 +50.0% 0.67x
Data.append.Sequence.64kB.Count0.I 134 200 +49.3% 0.67x
Data.init.Sequence.64kB.Count0 135 193 +43.0% 0.70x
Data.append.Sequence.64kB.Count0 135 191 +41.5% 0.71x
Data.append.Sequence.809B.Count0 198 274 +38.4% 0.72x
Data.init.Sequence.64kB.Count0.I 135 186 +37.8% 0.73x
Data.append.Sequence.809B.Count0.I 202 273 +35.1% 0.74x
UTF8Decode 255 342 +34.1% 0.75x
Data.init.Sequence.2049B.Count0.I 261 349 +33.7% 0.75x
Data.init.Sequence.2047B.Count0.I 259 345 +33.2% 0.75x
UTF8Decode_InitFromCustom_noncontiguous 304 401 +31.9% 0.76x
UTF8Decode_InitFromCustom_noncontiguous_ascii 761 994 +30.6% 0.77x
AngryPhonebook.ASCII2 109 142 +30.3% 0.77x
Data.init.Sequence.809B.Count.RE 47 61 +29.8% 0.77x
Dictionary4 155 200 +29.0% 0.78x
Data.init.Sequence.809B.Count.RE.I 47 59 +25.5% 0.80x
UTF8Decode_InitFromCustom_noncontiguous_ascii_as_ascii 872 1089 +24.9% 0.80x
Set.isDisjoint.Seq.Box0 492 608 +23.6% 0.81x
ObjectiveCBridgeStubDataAppend 1980 2380 +20.2% 0.83x (?)
ArrayPlusEqualSingleElementCollection 470 564 +20.0% 0.83x (?)
DropLastCountableRangeLazy 5 6 +20.0% 0.83x (?)
Set.isDisjoint.Seq.Box25 363 434 +19.6% 0.84x
Data.init.Sequence.809B.Count0.I 244 288 +18.0% 0.85x (?)
Set.isDisjoint.Seq.Empty.Box 81 95 +17.3% 0.85x
Data.init.Sequence.511B.Count0.I 267 313 +17.2% 0.85x (?)
Data.init.Sequence.513B.Count0.I 267 312 +16.9% 0.86x (?)
Data.init.Sequence.809B.Count0 247 286 +15.8% 0.86x (?)
Data.init.Sequence.809B.Count0.RE.I 240 275 +14.6% 0.87x (?)
UTF8Decode_InitDecoding 146 167 +14.4% 0.87x (?)
RangeOverlapsClosedRange 49 56 +14.3% 0.88x (?)
Data.init.Sequence.64kB.Count0.RE 134 153 +14.2% 0.88x
Data.init.Sequence.64kB.Count0.RE.I 135 154 +14.1% 0.88x
Data.init.Sequence.809B.Count0.RE 240 273 +13.7% 0.88x (?)
UTF8Decode_InitFromCustom_contiguous 147 167 +13.6% 0.88x (?)
ArrayInClass 900 1020 +13.3% 0.88x
StrToInt 1140 1290 +13.2% 0.88x
ParseInt.IntSmall.UncommonRadix 351 394 +12.3% 0.89x (?)
ParseInt.IntSmall.Decimal 321 359 +11.8% 0.89x
SuffixAnySequence 1836 2043 +11.3% 0.90x
DropLastAnySeqCntRange 441 487 +10.4% 0.91x (?)
Set.isDisjoint.Seq.Int100 116 128 +10.3% 0.91x (?)
Set.isSubset.Seq.Empty.Int 80 88 +10.0% 0.91x
Set.isDisjoint.Seq.Empty.Int 84 92 +9.5% 0.91x (?)
ConvertFloatingPoint.MockFloat64ToInt64 529 579 +9.5% 0.91x (?)
ParseInt.UInt64.Hex 373 406 +8.8% 0.92x
Set.isSuperset.Seq.Box0 194 211 +8.8% 0.92x (?)
DictionarySwap 612 664 +8.5% 0.92x (?)
NormalizedIterator_ascii 83 90 +8.4% 0.92x (?)
Set.isDisjoint.Empty.Int 86 93 +8.1% 0.92x
Data.append.Sequence.64kB.Count0.RE.I 136 147 +8.1% 0.93x (?)
Set.isDisjoint.Empty.Box 88 95 +8.0% 0.93x
Set.isSuperset.Seq.Int.Empty 88 95 +8.0% 0.93x
Set.isSuperset.Seq.Int0 119 128 +7.6% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
DropLastAnySequence 1767 477 -73.0% 3.70x
ConvertFloatingPoint.MockFloat64Exactly2 16 6 -62.5% 2.67x
DropFirstArrayLazy 26 13 -50.0% 2.00x
DropFirstArray 26 14 -46.2% 1.86x
ArrayAppendRepeatCol 850 510 -40.0% 1.67x
DataCreateMedium 6300 4100 -34.9% 1.54x
DataCountSmall 19 15 -21.1% 1.27x
SuffixCountableRangeLazy 5 4 -20.0% 1.25x (?)
DictionaryRemove 3910 3390 -13.3% 1.15x (?)
StringComparison_ascii 406 358 -11.8% 1.13x
ReversedBidirectional 13307 11819 -11.2% 1.13x
PrefixAnySeqCRangeIter 120 107 -10.8% 1.12x
PrefixAnySeqCntRange 120 107 -10.8% 1.12x
CharIndexing_ascii_unicodeScalars_Backwards 6680 5960 -10.8% 1.12x (?)
DataSubscriptSmall 19 17 -10.5% 1.12x (?)
DropFirstAnySeqCRangeIterLazy 143 128 -10.5% 1.12x (?)
CharIndexing_tweet_unicodeScalars_Backwards 13160 11800 -10.3% 1.12x (?)
AngryPhonebook.Strasse 142 128 -9.9% 1.11x (?)
DropFirstAnySeqCntRangeLazy 143 129 -9.8% 1.11x (?)
DropWhileAnySeqCRangeIterLazy 184 166 -9.8% 1.11x (?)
DropWhileAnySeqCntRangeLazy 184 166 -9.8% 1.11x (?)
SuffixAnyCollection 41 37 -9.8% 1.11x (?)
PrefixAnySeqCRangeIterLazy 134 121 -9.7% 1.11x
PrefixAnySeqCntRangeLazy 134 121 -9.7% 1.11x (?)
ArrayOfPOD 370 335 -9.5% 1.10x (?)
NibbleSort 2500 2270 -9.2% 1.10x (?)
PrefixAnyCollection 113 103 -8.8% 1.10x (?)
PrefixWhileAnyCollection 140 128 -8.6% 1.09x (?)
AngryPhonebook.Armenian 155 143 -7.7% 1.08x (?)
AngryPhonebook.Cyrillic 167 155 -7.2% 1.08x (?)
RemoveWhereSwapInts 15 14 -6.7% 1.07x (?)

Code size: -Osize

Regression OLD NEW DELTA RATIO
Suffix.o 19988 24709 +23.6% 0.81x
StaticArray.o 10825 11525 +6.5% 0.94x
StringRemoveDupes.o 4374 4519 +3.3% 0.97x
IterateData.o 1724 1777 +3.1% 0.97x
StrToInt.o 3360 3431 +2.1% 0.98x
RangeReplaceableCollectionPlusDefault.o 4305 4376 +1.6% 0.98x
DictTest.o 12034 12208 +1.4% 0.99x
PrimsNonStrongRef.o 92797 93990 +1.3% 0.99x
DropFirst.o 15891 16085 +1.2% 0.99x
DictTest3.o 13336 13475 +1.0% 0.99x
 
Improvement OLD NEW DELTA RATIO
ArrayOfPOD.o 3117 2904 -6.8% 1.07x
RecursiveOwnedParameter.o 1300 1232 -5.2% 1.06x
DropLast.o 19687 19069 -3.1% 1.03x
MonteCarloE.o 2702 2624 -2.9% 1.03x
Walsh.o 4018 3953 -1.6% 1.02x
Prefix.o 16287 16038 -1.5% 1.02x
ArrayInClass.o 4359 4296 -1.4% 1.01x
DiffingMyers.o 6178 6090 -1.4% 1.01x

Performance: -Onone

Regression OLD NEW DELTA RATIO
AngryPhonebook.ASCII2 110 144 +30.9% 0.76x
ArrayAppendLatin1 19754 24820 +25.6% 0.80x (?)
ArrayAppendAscii 19788 24684 +24.7% 0.80x
ArrayAppendAsciiSubstring 39384 48420 +22.9% 0.81x
ArrayAppendUTF16 19754 24072 +21.9% 0.82x
PopFrontArrayGeneric 3840 4660 +21.4% 0.82x (?)
ArrayAppendUTF16Substring 39312 47664 +21.2% 0.82x
ArrayAppendLatin1Substring 39888 48060 +20.5% 0.83x
ObjectiveCBridgeStubDataAppend 2540 3040 +19.7% 0.84x
UTF8Decode_InitFromCustom_contiguous 160 178 +11.2% 0.90x
UTF8Decode_InitDecoding 158 175 +10.8% 0.90x (?)
ObjectiveCBridgeStubDateAccess 4649 5033 +8.3% 0.92x (?)
ArrayInClass 2705 2915 +7.8% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
StringRemoveDupes 727 643 -11.6% 1.13x (?)
Dict.CopyKeyValue.16k 3589 3263 -9.1% 1.10x (?)
AngryPhonebook.Strasse 140 128 -8.6% 1.09x (?)
AngryPhonebook.Armenian 156 143 -8.3% 1.09x (?)
TwoSum 2802 2586 -7.7% 1.08x (?)
AngryPhonebook.Cyrillic 166 155 -6.6% 1.07x (?)

Code size: -swiftlibs

Improvement OLD NEW DELTA RATIO
libswiftFoundation.dylib 1327104 1310720 -1.2% 1.01x
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

atrick added 3 commits October 20, 2020 10:02
@atrick
Improve SimplifyCFG: remove conditional branches to the same target.
1d2d9b0
@atrick
SimplifyCFG: avoid introducing new critical edges.
628b537 
Passes will end up resplitting them, forcing SimplifyCFG to
continually rerun.

Also, we want to allow SIL utilities to assume no critical edges, and
avoid the need for several passes to internally split edges and modify
the CFG for no reason.
@atrick
SILInliner: Unconditional branches are essentially free.
b69b349
@atrick
Copy link
Contributor Author

atrick commented Oct 20, 2020

All tests passed, but the performance regressions are still blocked on my fixes so semantic call inlining from a year ago. I'll see if I can get those fixes checked in first.

@atrick atrick force-pushed the simplify-simplifycfg branch from 17490d0 to b69b349 Compare October 20, 2020 23:30
eeckstein
eeckstein approved these changes Oct 21, 2020
View reviewed changes
Copy link
Contributor

@eeckstein eeckstein left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm!

Do you know the reason for the benchmark regressions?

@atrick
Copy link
Contributor Author

atrick commented Oct 26, 2020

@eeckstein I've been investigating these regressions for the past week and attempting to stabilize the inliner/pipeline to avoid this sort of thing. They all look like arbitrary consequences of slight inlining changes. Unfortunately, any change I make to stabilize inlining requires other changes to avoid more regressions up to the point where the pass pipeline needs to be rewritten.

Many of the regressions are in Data.init/append. The executed code does not change at all here. It's a 6-instruction loop in Sequence._copyContents that appears extremely sensitive to code placement. I see these benchmarks swing one way or the other whenever I make a small change to inlining costs.

Instead of this PR, I have a series of smaller commits so it's easier to see where regressions first showed up.

@atrick atrick mentioned this pull request Oct 26, 2020
Merged
@atrick
Copy link
Contributor Author

atrick commented Oct 29, 2020

#34502 should avoid all the regressions

@atrick atrick closed this Oct 29, 2020
@atrick atrick deleted the simplify-simplifycfg branch November 9, 2020 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eeckstein eeckstein eeckstein approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@atrick @swift-ci @eeckstein