Skip to content

SILGen: Emit a closure literal in a function conversion as the converted type. #58651

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

Conversation

jckarter
Copy link
Contributor

@jckarter jckarter commented May 3, 2022

Closure literals are sometimes type-checked as one type then immediately converted to another
type in the AST. One particular case of this is when a closure body never throws, but the closure
is used as an argument to a function that takes a parameter that throws. Emitting this naively,
by emitting the closure as its original type, then converting to throws, can be expensive for
async closures, since that takes a reabstraction thunk. Even for non-async functions, we still want
to get the benefit of reabstraction optimization for the closure literal through the conversion too.
So if the function conversion just add throws, emit the closure as throwing, and pass down the
context abstraction pattern when emitting the closure as well.

@jckarter
Copy link
Contributor Author

jckarter commented May 3, 2022

@swift-ci Please test

@jckarter
Copy link
Contributor Author

jckarter commented May 3, 2022

@swift-ci Please benchmark

Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

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

LGTM

//
// TODO: Move this up when we can emit closures directly with C calling
// convention.
if (isa<AbstractClosureExpr>(e->getSubExpr())
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can at least do getSemanticsProvidingExpr() here

@jckarter
Copy link
Contributor Author

jckarter commented May 4, 2022

@swift-ci Please benchmark

@drexin
Copy link
Contributor

drexin commented May 4, 2022

Performance (x86_64): -O

Regression OLD NEW DELTA RATIO
Breadcrumbs.IdxToUTF16Range.longMixed 235 377 +60.4% 0.62x (?)
Breadcrumbs.IdxToUTF16.longMixed 457 690 +51.0% 0.66x
NormalizedIterator_fastPrenormal 430 540 +25.6% 0.80x (?)
NormalizedIterator_emoji 300 364 +21.3% 0.82x (?)
NormalizedIterator_latin1 150 180 +20.0% 0.83x (?)
ObjectiveCBridgeStubDateAccess 133 152 +14.3% 0.88x
ObjectiveCBridgeStringIsEqual2 149 170 +14.1% 0.88x (?)
ObjectiveCBridgeStringIsEqualAllSwift 46 52 +13.0% 0.88x (?)
ObjectiveCBridgeStringIsEqual 135 152 +12.6% 0.89x (?)
Set.isDisjoint.Seq.Box.Empty 80 90 +12.5% 0.89x (?)
SubstringEquatable 269 302 +12.3% 0.89x (?)
OpenClose 52 58 +11.5% 0.90x (?)
StringEnumRawValueInitialization 360 400 +11.1% 0.90x (?)
Set.isSuperset.Seq.Empty.Int 45 50 +11.1% 0.90x (?)
ObjectiveCBridgeStringHash 67 74 +10.4% 0.91x (?)
PrefixWhileAnySequence 205 226 +10.2% 0.91x (?)
ObjectiveCBridgeStringGetASCIIContents 246 269 +9.3% 0.91x (?)
FlattenListLoop 937 1023 +9.2% 0.92x (?)
DictionaryBridgeToObjC_BulkAccess 93 101 +8.6% 0.92x (?)
 
Improvement OLD NEW DELTA RATIO
SortLargeExistentials 39200 14500 -63.0% 2.70x
Data.append.Sequence.64kB.Count.RE.I 29 21 -27.6% 1.38x
Data.append.Sequence.64kB.Count.RE 29 21 -27.6% 1.38x
LessSubstringSubstring 28 23 -17.9% 1.22x
EqualSubstringSubstring 28 23 -17.9% 1.22x (?)
EqualStringSubstring 28 23 -17.9% 1.22x (?)
EqualSubstringSubstringGenericEquatable 28 23 -17.9% 1.22x (?)
EqualSubstringString 28 23 -17.9% 1.22x (?)
LessSubstringSubstringGenericComparable 28 23 -17.9% 1.22x (?)
Data.append.Sequence.809B.Count.RE.I 68 58 -14.7% 1.17x (?)
StringUTF16Builder 220 190 -13.6% 1.16x (?)
Data.append.Sequence.809B.Count.RE 67 58 -13.4% 1.16x (?)
StringBuilder 207 180 -13.0% 1.15x (?)
StringBuilderSmallReservingCapacity 215 187 -13.0% 1.15x (?)
String.replaceSubrange.String 17 15 -11.8% 1.13x (?)
StringComparison_longSharedPrefix 330 294 -10.9% 1.12x (?)
StringAdder 268 241 -10.1% 1.11x (?)
InsertCharacterEndIndexNonASCII 44 40 -9.1% 1.10x (?)
InsertCharacterTowardsEndIndex 144 131 -9.0% 1.10x (?)
FatCompactMap 570 520 -8.8% 1.10x (?)
InsertCharacterEndIndex 129 119 -7.8% 1.08x (?)

Code size: -O

Regression OLD NEW DELTA RATIO
AngryPhonebook.o 8522 9046 +6.1% 0.94x
RangeReplaceableCollectionPlusDefault.o 6396 6572 +2.8% 0.97x
StringEdits.o 8731 8971 +2.7% 0.97x
ChainedFilterMap.o 3157 3237 +2.5% 0.98x
Breadcrumbs.o 43923 44787 +2.0% 0.98x
DictionaryBridge.o 3204 3252 +1.5% 0.99x
StrToInt.o 5408 5472 +1.2% 0.99x
 
Improvement OLD NEW DELTA RATIO
SortLargeExistentials.o 22188 20348 -8.3% 1.09x
IterateData.o 1872 1784 -4.7% 1.05x
FloatingPointParsing.o 13746 13495 -1.8% 1.02x
RandomTree.o 12079 11911 -1.4% 1.01x
ObjectiveCNoBridgingStubs.o 8076 7988 -1.1% 1.01x

Performance (x86_64): -Osize

Regression OLD NEW DELTA RATIO
FlattenListLoop 965 1582 +63.9% 0.61x (?)
Breadcrumbs.IdxToUTF16Range.longMixed 235 377 +60.4% 0.62x (?)
Breadcrumbs.IdxToUTF16.longMixed 464 692 +49.1% 0.67x
ParseInt.UInt64.Hex 272 329 +21.0% 0.83x (?)
NormalizedIterator_nonBMPSlowestPrenormal 390 460 +17.9% 0.85x (?)
NormalizedIterator_emoji 316 372 +17.7% 0.85x (?)
NormalizedIterator_fastPrenormal 460 530 +15.2% 0.87x (?)
ObjectiveCBridgeStringIsEqual2 146 166 +13.7% 0.88x (?)
ObjectiveCBridgeStringIsEqual 133 151 +13.5% 0.88x (?)
ObjectiveCBridgeStringIsEqualAllSwift 45 51 +13.3% 0.88x (?)
SubstringEquatable 270 305 +13.0% 0.89x (?)
NormalizedIterator_slowerPrenormal 270 300 +11.1% 0.90x (?)
ObjectiveCBridgeStringHash 67 74 +10.4% 0.91x (?)
NSStringConversion.MutableCopy.Rebridge.LongUTF8 328 361 +10.1% 0.91x (?)
Set.isDisjoint.Empty.Box 82 90 +9.8% 0.91x (?)
ObjectiveCBridgeStringGetASCIIContents 246 269 +9.3% 0.91x (?)
Set.isStrictSubset.Int0 75 82 +9.3% 0.91x (?)
ParseInt.UInt64.Decimal 129 139 +7.8% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
SortLargeExistentials 41800 14600 -65.1% 2.86x
DataAccessBytesMedium 59 45 -23.7% 1.31x
StringBuilder 218 178 -18.3% 1.22x (?)
LessSubstringSubstring 28 23 -17.9% 1.22x (?)
EqualSubstringSubstring 28 23 -17.9% 1.22x (?)
EqualStringSubstring 28 23 -17.9% 1.22x
EqualSubstringSubstringGenericEquatable 28 23 -17.9% 1.22x (?)
EqualSubstringString 28 23 -17.9% 1.22x (?)
LessSubstringSubstringGenericComparable 28 23 -17.9% 1.22x (?)
StringAdder 268 221 -17.5% 1.21x
DataSubscriptSmall 19 16 -15.8% 1.19x (?)
StringBuilderSmallReservingCapacity 219 187 -14.6% 1.17x (?)
StringUTF16Builder 220 190 -13.6% 1.16x (?)
DataMutateBytesSmall 160 140 -12.5% 1.14x (?)
String.replaceSubrange.String 17 15 -11.8% 1.13x (?)
StringComparison_longSharedPrefix 333 294 -11.7% 1.13x (?)
DataCountSmall 19 17 -10.5% 1.12x
DataCountMedium 19 17 -10.5% 1.12x (?)
Data.hash.Empty 58 52 -10.3% 1.12x (?)
StringInterpolationSmall 1220 1110 -9.0% 1.10x (?)
SortStringsUnicode 2140 1955 -8.6% 1.09x (?)
ArrayAppendAsciiSubstring 12960 11988 -7.5% 1.08x (?)
ArrayAppendLatin1Substring 13176 12204 -7.4% 1.08x (?)
ArrayAppendUTF16Substring 12924 11988 -7.2% 1.08x (?)
DictionaryKeysContainsCocoa 15 14 -6.7% 1.07x (?)
DropWhileSequence 15 14 -6.7% 1.07x (?)
DictionaryKeysContainsNative 15 14 -6.7% 1.07x (?)

Code size: -Osize

Regression OLD NEW DELTA RATIO
AngryPhonebook.o 8023 8565 +6.8% 0.94x
IntegerParsing.o 53563 55981 +4.5% 0.96x
Breadcrumbs.o 36188 37027 +2.3% 0.98x
DataBenchmarks.o 48968 49939 +2.0% 0.98x
RangeReplaceableCollectionPlusDefault.o 6332 6452 +1.9% 0.98x
ChainedFilterMap.o 2905 2954 +1.7% 0.98x
SetTests.o 121628 123079 +1.2% 0.99x
DiffingMyers.o 6631 6708 +1.2% 0.99x
 
Improvement OLD NEW DELTA RATIO
SortLargeExistentials.o 22854 21036 -8.0% 1.09x
StrToInt.o 5279 4895 -7.3% 1.08x
ObjectiveCNoBridgingStubs.o 7542 7113 -5.7% 1.06x
IterateData.o 1632 1548 -5.1% 1.05x
ObjectiveCBridgingStubs.o 17974 17336 -3.5% 1.04x
RandomTree.o 11341 11125 -1.9% 1.02x
DictionaryBridge.o 3204 3144 -1.9% 1.02x
StringInterpolation.o 6695 6594 -1.5% 1.02x

Performance (x86_64): -Onone

Regression OLD NEW DELTA RATIO
SIMDReduce.Int8x64.Cast 1805 2734 +51.5% 0.66x
Breadcrumbs.IdxToUTF16Range.longMixed 335 482 +43.9% 0.70x
SIMDReduce.Int32x16.Cast 814 1059 +30.1% 0.77x
SIMDReduce.Int8x16.Cast 892 1135 +27.2% 0.79x (?)
Breadcrumbs.IdxToUTF16.longMixed 922 1167 +26.6% 0.79x (?)
ObjectiveCBridgeStringIsEqual2 147 166 +12.9% 0.89x (?)
ObjectiveCBridgeStringIsEqual 133 150 +12.8% 0.89x (?)
ObjectiveCBridgeStringIsEqualAllSwift 45 50 +11.1% 0.90x (?)
ObjectiveCBridgeStringHash 67 74 +10.4% 0.91x (?)
NormalizedIterator_nonBMPSlowestPrenormal 400 440 +10.0% 0.91x (?)
SIMDReduce.Int32x4.Cast 648 709 +9.4% 0.91x (?)
NormalizedIterator_fastPrenormal 470 510 +8.5% 0.92x (?)
 
Improvement OLD NEW DELTA RATIO
Set.filter.Int100.24k 173 128 -26.0% 1.35x
Set.filter.Int100.16k 116 86 -25.9% 1.35x
Set.filter.Int100.28k 205 152 -25.9% 1.35x (?)
Set.filter.Int100.20k 144 107 -25.7% 1.35x
EqualSubstringSubstringGenericEquatable 59 55 -6.8% 1.07x (?)
Set.filter.Int50.24k 653 610 -6.6% 1.07x (?)

Code size: -swiftlibs

…ted type.

Closure literals are sometimes type-checked as one type then immediately converted to another
type in the AST. One particular case of this is when a closure body never throws, but the closure
is used as an argument to a function that takes a parameter that `throws`. Emitting this naively,
by emitting the closure as its original type, then converting to throws, can be expensive for
async closures, since that takes a reabstraction thunk. Even for non-async functions, we still want
to get the benefit of reabstraction optimization for the closure literal through the conversion too.
So if the function conversion just add `throws`, emit the closure as throwing, and pass down the
context abstraction pattern when emitting the closure as well.
@jckarter jckarter force-pushed the closure-literal-function-conversion-throws branch from 4a741fb to 482cc8a Compare May 4, 2022 17:24
@jckarter
Copy link
Contributor Author

jckarter commented May 4, 2022

@swift-ci Please test

@jckarter jckarter merged commit 55cda87 into swiftlang:main May 4, 2022
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.

3 participants