4.2 SILCombiner::propagateConcreteTypeOfInitExistential fixes. #17554
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
…ial.cpp/h in order for other passes such as ExistentialSpecializer to use it apart from SILCombiner
In an upcoming bug fix, I want to pass SILBuilderContext to a utility. I could continue reusing SILBuilder, even though the utility must set its own insertion point and debug location. However, this is terrible practice that I don't want to perpetuate. The lifetime of a SILBuilder should correspond to a single insertion point and debug location. That's the only sane way to preserve debug information in SIL passes. There are various pieces of contextual state that we've been adding to the SILBuilder. Those have made it impossible to use SILBuilder correctly. I'm pulling the context out, so clients can begin using better practices. In the future, we can make SILBuilderContext polymorphic, so passes can extend it easily with arbitrary callbacks. We can also make it self-contained so we don't need to pass around pointers to an InsertedInst list anymore. (cherry picked from commit df94dff)
Fixes <rdar://40555427> [SR-7773]: SILCombiner::propagateConcreteTypeOfInitExistential fails to full propagate type substitutions. Fixes <rdar://problem/40923849> SILCombiner::propagateConcreteTypeOfInitExistential crashes on protocol compositions. This rewrite fixes several fundamental bugs in the SILCombiner optimization that propagates concrete types. In particular, the pass needs to handle: - Arguments of callee Self type in non-self position. - Indirect and direct return values of Self type. - Types that indirectly depend on Self within callee function signature. - Protocol composition existentials. - All of the above need to work for protocol extensions as well as witness methods. - For protocol extensions, conformance lookup should be based on the existential's conformance list. Additionally, the optimization should not depend on a SILFunction's DeclContext, which is not serialized. (In fact, we should prevent SIL passes from using DeclContext). Furthermore, the code needs to be expressed in a way that one can reason about correctness and invariants. The root cause of these bugs is that SIL passes are written based on untested assumptions of Swift type system. A SIL pass needs to handle all verifiable SIL input because passes need to be composable. Bail-out logic can be added to simplify the design; however, _the bail-out logic itself cannot make any assumptions about the language or type system_ that aren't clearly and explicitly enforced in the SIL verifier. This is a common mistake and major source of bugs. I created as many unit tests as I reasonably could to prevent this code from regressing. Creating enough unit tests to cover all corner cases that were broken in the original code would be intractable. But the code has been simplified such that many corner cases disappear. This opens up some oportunity for generalizing the optimization and eliminating special cases. However, I want this PR to be limited to fixing correctness issues only. In the long term, it would be preferable to replace this optimization entirely with a much more powerful general type propagation pass. (cherry picked from commit 9d4b4c7)
|
@swift-ci test. |
|
@slavapestov The last commit has the 4.2 vs master merge: Don't worry, there are no wayward calls to |
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.
Merge SILCombine fixes onto 4.2.
Some simple low-risk NFC changes are merged in order to localize the important higher-risk differences between 4.2 and master. Some of the new code required finessing into 4.2, mainly because the SIL-level SubstitionMaps weren't merged.