Add value witnesses for single payload enums #12606
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
So far single payload enums were implemented in terms of runtime functions which internally emitted several calls to value witnesses. This commit adds value witnesses to get and store the enum tag side stepping the need for witness calls as this information is statically available in many cases /// int (*getEnumTagSinglePayload)(const T* enum, UINT_TYPE emptyCases) /// Given an instance of valid single payload enum with a payload of this /// witness table's type (e.g Optional<ThisType>) , get the tag of the enum. /// void (*storeEnumTagSinglePayload)(T* enum, INT_TYPE whichCase, /// UINT_TYPE emptyCases) /// Given uninitialized memory for an instance of a single payload enum with a /// payload of this witness table's type (e.g Optional<ThisType>), store the /// tag. A simple 'for element in array' loop in generic code operating on a ContigousArray of Int is ~25% faster on arm64. rdar://31408033
Fast-isel does not like them
|
@swift-ci Please test |
|
Build failed |
|
@swift-ci Please test linux |
|
What's the code size impact for this change? |
|
I don’t have numbers yet. it should somewhat even out with the removal of the array witnesses (of which there where four) |
|
@swift-ci Please benchmark |
slavapestov
reviewed
Oct 25, 2017
There was a problem hiding this comment.
It looks like the extra inhabitant value witnesses are now only used when you have an enum whose payload is a structural type, like a tuple. Can you think of a clever way around this, implementing the single payload value witnesses for tuples in terms of the single payload value witnesses of the payload type? If so, you could trim the value witness table further.
lib/IRGen/GenType.cpp
Outdated
| // numTags += ((emptyCases + (casesPerTagBitValue - 1U)) >> bits); | ||
| // } | ||
| // return (numTags < 256 ? 1 : | ||
| // numTags < 65536 ? 2 : 4); |
lib/IRGen/GenType.cpp
Outdated
| auto *phi = Builder.CreatePHI(int32Ty, 3); | ||
| phi->addIncoming(one, entryBB); | ||
| phi->addIncoming(numTags, notLT256BB); | ||
| return phi; |
|
For example, would it be sufficient to say that
This relies on the fact that extra inhabitants of a tuple type are always the extra inhabitants of its first component. If this were to change, the above would no longer work, but maybe we can live with that restriction? |
Build comment file:Optimized (O)Regression (9)
Improvement (14)
No Changes (311)
Unoptimized (Onone)Regression (3)
Improvement (60)
No Changes (271)
Hardware Overview |
|
@slavapestov They are also used for non-fixed size structs and enums. Where the enums are the hairy part. structs currently follow the same rule as tuples (first field's extra inhabitants).
If we are okay to loose the single payload enums extra inhabitants I believe the scheme would work. A further complication is that now the enum witnesses would have to be described in terms of an abstract size such that we can find the address of the extra tag bytes when we forward to the payload of an enum/first element of a struct/tuple . This is doable (see below) but means that the witnesses of fixed size types will no longer simplify as well because the size is dynamic. The witnesses would get an additional argument that is the size of the 'outermost' type that we append any potential . would become We pass null at the top-level of witness calls. So fixed size types will still use the static size in the common case. Hand-wavy sketch for a fixed size type: The witness for a tuple will look like: What does that mean for code size: We have the cost of passing an extra zero at all call-sites plus bigger witness implementations for fixed size types because the size is dynamic. Not immediately clear that this is a code size win. You only pay for extra inhabitant witnesses if there are extra inhabitants. |
|
Ok, we can stick with the current approach then. |
Conditionally (on Osize) outline the witness call - this saves two loads
|
The standard library text size increases by 0.9% on x86-64. From 4254426 to 4292298 bytes. Out of the 37K bytes, 21K bytes are from the witnesses, the rest is overhead because we now have to load the witness on the caller side. |
|
(Numbers on arm are similar) |
|
@swift-ci Please smoke test mac os x |
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
@swift-ci Please test linux |
|
@swift-ci Please benchmark |
Build comment file:Optimized (O)Regression (10)
Improvement (8)
No Changes (316)
Unoptimized (Onone)Regression (6)
Improvement (51)
No Changes (277)
Hardware Overview |
|
Code size swift-source-compat-suite. On interesting fact I noticed while looking at: that the delta (3.3%) could roughly be half that. There are duplicate value witnesses that llvm's merge function pass would merge. We run -wmo -num-threads 1..N and so these witnesses are split up per source file and don't get merged. That is -wmo -num-threads >0 is not helpful for the merge function pass and we leave code size improvements on the table (not only for witnesses). This is the obvious trade-off of num-threads but had not occurred to me before. |
eeckstein
added a commit
that referenced
this pull request
Oct 31, 2017
Revert "Merge pull request #12606 from aschwaighofer/single_payload_enum_witness"
aschwaighofer
added a commit
to aschwaighofer/swift
that referenced
this pull request
Nov 1, 2017
Revert "Revert "Merge pull request swiftlang#12606 from aschwaighofer/single_payload_enum_witness"" This reverts commit c422f80.
rjmccall
added a commit
to rjmccall/swift
that referenced
this pull request
Dec 12, 2018
This is essentially a long-belated follow-up to Arnold's swiftlang#12606. The key observation here is that the enum-tag-single-payload witnesses are strictly more powerful than the XI witnesses: you can simulate the XI witnesses by using an extra case count that's <= the XI count. Of course the result is less efficient than the XI witnesses, but that's less important than overall code size, and we can work on fast-paths for that. The extra inhabitant count is stored in a 32-bit field (always present) following the ValueWitnessFlags, which now occupy a fixed 32 bits. This inflates non-XI VWTs on 32-bit targets by a word, but the net effect on XI VWTs is to shrink them by two words, which is likely to be the more important change. Also, being able to access the XI count directly should be a nice win.
rjmccall
added a commit
to rjmccall/swift
that referenced
this pull request
Dec 12, 2018
This is essentially a long-belated follow-up to Arnold's swiftlang#12606. The key observation here is that the enum-tag-single-payload witnesses are strictly more powerful than the XI witnesses: you can simulate the XI witnesses by using an extra case count that's <= the XI count. Of course the result is less efficient than the XI witnesses, but that's less important than overall code size, and we can work on fast-paths for that. The extra inhabitant count is stored in a 32-bit field (always present) following the ValueWitnessFlags, which now occupy a fixed 32 bits. This inflates non-XI VWTs on 32-bit targets by a word, but the net effect on XI VWTs is to shrink them by two words, which is likely to be the more important change. Also, being able to access the XI count directly should be a nice win.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So far single payload enums were implemented in terms of runtime functions which
internally emitted several calls to value witnesses.
This commit adds value witnesses to get and store the enum tag side stepping the
need for witness calls as this information is statically available in many cases
/// int (getEnumTagSinglePayload)(const T enum, UINT_TYPE emptyCases)
/// Given an instance of valid single payload enum with a payload of this
/// witness table's type (e.g Optional) , get the tag of the enum.
/// void (storeEnumTagSinglePayload)(T enum, INT_TYPE whichCase,
/// UINT_TYPE emptyCases)
/// Given uninitialized memory for an instance of a single payload enum with a
/// payload of this witness table's type (e.g Optional), store the
/// tag.
A simple 'for element in array' loop in generic code operating on a
ContiguousArray of Int is ~25% faster on arm64.
rdar://31408033