Skip to content

Conversation

@nate-chandler
Copy link
Contributor

When metadata record for a generic type is locally cached as ::Complete, the metadata is known to be complete in that scope. If it's for a generic type, being complete means that all of its recursive generic arguments are also complete.

Previously, though, that fact was not exploited. So a metadata record for such an argument which was originally obtained via an incomplete request would remain cached in that incomplete state even when a generic type in which it appeared as a generic argument was cached as complete. At worst, the result was a runtime call (checkMetadataState) to promote the complete the metadata record for the recursive argument.

Here, when a bound generic type's metadata is locally cached as complete, its recursive generic arguments are visited; for each such type for which a metadata record is already locally cached, the preexisting record is recached as complete.

@nate-chandler nate-chandler changed the title [IRGen] Propagate metadata completion to generic arguments. [IRGen] Propagate locally-cached metadata completion to generic arguments. Oct 25, 2023
@nate-chandler nate-chandler marked this pull request as ready for review October 25, 2023 22:10
@nate-chandler nate-chandler force-pushed the irgen/note-implicitly-completed-metadata branch 3 times, most recently from 4ca6279 to 8a7179e Compare October 26, 2023 23:19
@nate-chandler
Copy link
Contributor Author

@swift-ci please apple silicon benchmark

@nate-chandler
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. I think this is going to force abstract entries in the LocalTypeData cache to emitted (abstract in the sense of the LTDC, i.e. metadata that can cheaply be produced). We probably don't want that; it might have a lot of extra compile-time cost, even if LLVM can eliminate it later.

It might be easiest to just add a new operation on the LTDC, some sort of update-state-in-scope operation. The initial implementation can just affect concrete entries, but in principle we could also make this work with abstract entries by cloning the abstract path and then telling it to override the completeness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Added an advanceStateInScope operation to LTDC. It looks for relevant cache entries and updates the cache accordingly. There are TODOs for the abstract case. If a relevant concrete entry is found (i.e. one whose dominance point dominates the active dominance point), it's used to produce a new cache entry whose dominance point is the active dominance point and whose state is the one specified.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's sortof implied by the code, but please update the comment to talk about transitively-complete metadata rather than the vaguer "all of the metadata it references".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

When metadata record for a generic type is locally cached as ::Complete,
the metadata is known to be complete in that scope.  If it's for a
generic type, being complete means that all of its recursive generic
arguments are also complete.

Previously, though, that fact was not exploited.  So a metadata record
for such an argument which was originally obtained via an incomplete
request would remain cached in that incomplete state even when a generic
type in which it appeared as a generic argument was cached as complete.
At worst, the result was a runtime call (checkMetadataState) to promote
the complete the metadata record for the recursive argument.

Here, when a bound generic type's metadata is locally cached as
complete, its recursive generic arguments are visited; for each such
type for which a metadata record is already locally cached, the
preexisting record is recached as complete.
@nate-chandler nate-chandler force-pushed the irgen/note-implicitly-completed-metadata branch from 8a7179e to b8990b5 Compare October 27, 2023 19:08
@nate-chandler
Copy link
Contributor Author

@swift-ci please apple silicon benchmark

@nate-chandler
Copy link
Contributor Author

@swift-ci please test

@nate-chandler
Copy link
Contributor Author

### Performance (arm64): -O

**Improvement**                       | **OLD** | **NEW** | **DELTA** | **RATIO**
:---                                  | ---:    | ---:    | ---:   | ---:     
Data.init.Sequence.511B.Count.I       | 19.648  | 17.384  | -11.5% | **1.13x (?)**
BufferFillFromSlice                   | 12.107  | 10.793  | -10.9% | **1.12x (?)**
Breadcrumbs.IdxToUTF16Range.longASCII | 10.308  | 9.37    | -9.1%  | **1.10x (?)**
ArrayAppendGenericStructs             | 977.5   | 891.111 | -8.8%  | **1.10x (?)**
SubstringFromLongStringGeneric2       | 20.263  | 18.697  | -7.7%  | **1.08x (?)**
NSStringConversion.Rebridge           | 75.643  | 69.914  | -7.6%  | **1.08x (?)**
String.replaceSubrange.RepChar        | 943.0   | 876.0   | -7.1%  | **1.08x (?)**
NSStringConversion                    | 295.714 | 274.75  | -7.1%  | **1.08x (?)**
StringFromLongWholeSubstring          | 2.169   | 2.016   | -7.1%  | **1.08x**

### Code size: -O


### Performance (arm64): -Osize

**Regression**                        | **OLD** | **NEW** | **DELTA** | **RATIO**
:---                                  | ---:    | ---:    | ---:   | ---:     
ObserverForwarderStruct               | 193.696 | 210.8   | +8.8%  | **0.92x (?)**
  | | | | 
**Improvement**                       | **OLD** | **NEW** | **DELTA** | **RATIO**
StringWithCString2                    | 0.002   | 0.0     | -66.7% | **3.00x**
String.replaceSubrange.RepChar        | 949.0   | 860.0   | -9.4%  | **1.10x (?)**
Breadcrumbs.IdxToUTF16Range.longASCII | 11.554  | 10.62   | -8.1%  | **1.09x**
SubstringFromLongStringGeneric2       | 20.271  | 18.707  | -7.7%  | **1.08x (?)**
NSStringConversion.Rebridge           | 75.688  | 69.9    | -7.6%  | **1.08x (?)**
NSStringConversion                    | 295.667 | 274.667 | -7.1%  | **1.08x (?)**
StringFromLongWholeSubstring          | 2.328   | 2.172   | -6.7%  | **1.07x**

### Code size: -Osize


### Performance (arm64): -Onone

**Regression**                        | **OLD**  | **NEW**  | **DELTA** | **RATIO**
:---                                  | ---:     | ---:     | ---:   | ---:     
CharacterLiteralsLarge                | 230.75   | 262.0    | +13.5% | **0.88x (?)**
SIMDRandomMask.Int8x64                | 11013.0  | 11854.0  | +7.6%  | **0.93x (?)**
  | | | | 
**Improvement**                       | **OLD**  | **NEW**  | **DELTA** | **RATIO**
ArrayAppendGenericStructs             | 766.667  | 455.0    | -40.7% | **1.68x (?)**
RawBuffer.1000.findFirst              | 55503.0  | 47703.0  | -14.1% | **1.16x**
DataCreateMedium                      | 101300.0 | 88400.0  | -12.7% | **1.15x (?)**
RawBuffer.1000.findLast               | 61712.0  | 53889.0  | -12.7% | **1.15x**
RawBuffer.128.findLast                | 8449.0   | 7451.0   | -11.8% | **1.13x (?)**
RawBuffer.7.findLast                  | 1144.0   | 1011.5   | -11.6% | **1.13x (?)**
RawBuffer.39.findFirst                | 2813.0   | 2509.0   | -10.8% | **1.12x (?)**
RawBuffer.39.findLast                 | 3092.0   | 2785.0   | -9.9%  | **1.11x (?)**
DataCreateSmall                       | 13380.0  | 12150.0  | -9.2%  | **1.10x (?)**
PrefixWhileSequenceLazy               | 6420.0   | 5840.0   | -9.0%  | **1.10x (?)**
PrefixWhileAnySequenceLazy            | 6491.0   | 5914.0   | -8.9%  | **1.10x (?)**
DropWhileAnySequence                  | 8542.0   | 7800.0   | -8.7%  | **1.10x (?)**
DropWhileSequence                     | 8293.0   | 7579.0   | -8.6%  | **1.09x (?)**
DropWhileSequenceLazy                 | 7835.0   | 7165.0   | -8.6%  | **1.09x (?)**
RawBuffer.7.findFirst                 | 1020.0   | 933.0    | -8.5%  | **1.09x (?)**
DropWhileAnySequenceLazy              | 7906.0   | 7252.0   | -8.3%  | **1.09x (?)**
DictionaryCompactMapValuesOfNilValue  | 6658.333 | 6123.529 | -8.0%  | **1.09x (?)**
String.replaceSubrange.RepChar        | 946.0    | 872.0    | -7.8%  | **1.08x (?)**
NSStringConversion                    | 330.5    | 307.5    | -7.0%  | **1.07x (?)**
Dict.FilterAllMatch.24k               | 107.565  | 100.08   | -7.0%  | **1.07x (?)**
DropFirstSequence                     | 5524.0   | 5140.0   | -7.0%  | **1.07x (?)**
Dict.FilterAllMatch.20k               | 90.111   | 83.857   | -6.9%  | **1.07x (?)**
DropFirstSequenceLazy                 | 5517.0   | 5137.0   | -6.9%  | **1.07x (?)**
Dict.FilterAllMatch.16k               | 72.625   | 67.636   | -6.9%  | **1.07x (?)**
Dict.FilterAllMatch.28k               | 127.5    | 118.762  | -6.9%  | **1.07x (?)**
ObjectiveCBridgeStringIsEqualAllSwift | 32.138   | 30.03    | -6.6%  | **1.07x (?)**

### Code size: -swiftlibs

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.

This looks great!

@nate-chandler nate-chandler changed the title [IRGen] Propagate locally-cached metadata completion to generic arguments. [IRGen] Propagate locally-cached metadata completion to transitive dependencies. Oct 27, 2023
@nate-chandler nate-chandler merged commit 137ba61 into swiftlang:main Oct 28, 2023
@nate-chandler nate-chandler deleted the irgen/note-implicitly-completed-metadata branch October 28, 2023 00:13
@slavapestov
Copy link
Contributor

Nice!

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