[Type checker] Stop devirtualizing the reference to IteratorProtocol.next() #28984
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
…olver. The type checking of the for-each loop is split between the constraint solver (which does most of the work) and the statement checker (which updates the for-each loop AST). Move more of the work into the constraint solver proper, so that the AST updates can happen in one place, making use of the solution produced by the solver. This allows a few things, some of which are short-term gains and others that are more future-facing: * `TypeChecker::convertToType` has been removed, because we can now either use the more general `typeCheckExpression` entry point or perform the appropriate operation within the constraint system. * Solving the constraint system ensures that everything related to the for-each loop full checks out * Additional refactoring will make it easier for the for-each loop to be checked as part of a larger constraint system, e.g., for processing entire closures or function bodies (that’s the futurist bit).
Introduce a new kind of constraint, the "value witness" constraint, which captures a reference to a witness for a specific protocol conformance. It otherwise acts like a more restricted form of a "value member" constraint, where the specific member is known (as a ValueDecl*) in advance. The constraint is effectively dependent on the protocol conformance itself; if that conformance fails, mark the type variables in the resolved member type as "holes", so that the conformance failure does not cascade. Note that the resolved overload for this constraint always refers to the requirement, rather than the witness, so we will end up recording witness-method references in the AST rather than concrete references, and leave it up to the optimizers to perform devirtualization. This is demonstrated by the SIL changes needed in tests, and is part of the wider resilience issue with conformances described by rdar://problem/22708391.
…lution. While this doesn't completely use the solution's set of known conformances, it moves the logic for handling the lookup into the right place.
Rather than having the type checker form the ConcreteDeclRef for makeIterator, have SILGen do it, because it's fairly trivial. Eliminates some redundant state from the AST.
…next(). Rather than having the type checker look for the specific witness to next() when type checking the for-each loop, which had the effect of devirtualizing next() even when it shouldn't be, leave the formation of the next() reference to SILGen. There, form it as a witness reference, so that the SIL optimizer can choose whether to devirtualization (or not).
|
@swift-ci please smoke test |
|
@swift-ci please benchmark |
|
This is a follow-on to #28380 |
xedin
approved these changes
Jan 2, 2020
| recordPotentialHole(typeVar); | ||
| }); | ||
|
|
||
| return SolutionKind::Solved; |
There was a problem hiding this comment.
I think we need to record AddMissingConformance fix here otherwise we might produce a solution with holes but without any fixes which would crash later in either verifier (in debug build) or SILGen (in release).
There was a problem hiding this comment.
Huh, okay. I guess it's fine to have redundant missing-conformance fixes?
There was a problem hiding this comment.
Yeah, they going to get coalesced into one for the same locator.
Performance: -O
Code size: -OPerformance: -Osize
Code size: -OsizePerformance: -Onone
Code size: -swiftlibs
How to read the dataThe 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 Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
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.
Rather than having the type checker look for the specific witness to
next() when type checking the for-each loop, which had the effect of
devirtualizing next() even when it shouldn't be, leave the formation
of the next() reference to SILGen. There, form it as a witness
reference, so that the SIL optimizer can choose whether to
devirtualization (or not).