Rewrite SILCombiner::propagateConcreteTypeOfInitExistential. #17315
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
|
@slavapestov Normally I would not introduce an optimization rewrite late in a release branch. However, the rewritten code is lower risk than leaving any of the existing code in place. Removing the optimization altogether would introduce the risk of sudden regressions in a release that we don't have time to fix. The best way to review this I think is to checkout the branch and read through these functions (there's nothing in the original code worth looking at other than what was copied into the ConcreteExistentialInfo constructor): SILCombiner::createApplyWithConcreteType |
|
@rajbarik this might trivially conflict with your PRs. I left |
|
@swift-ci test. |
|
@swift-ci test source compatibility |
|
@swift-ci benchmark. |
Build comment file:Optimized (O)Improvement (1)
No Changes (445)
Unoptimized (Onone)Regression (7)
Improvement (4)
No Changes (435)
Hardware Overview |
|
The -Onone benchmark regressions and improvements are all spurious. None of this code is touched at -Onone. |
|
Thanks. I will take a look after it is merged.
…On Mon, Jun 18, 2018 at 5:19 PM, Andrew Trick ***@***.***> wrote:
@rajbarik <https://github.com/rajbarik> this might trivially conflict
with your PRs. I left findInitExistential in the code for your
convenience, but needed to slightly change the signature. If
ConcreteExistentialInfo can be used in all cases instead, then we could
remove it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17315 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AFBwaWEZW_MVdYX7XEn8vV2rq5E_-u3Mks5t-EOQgaJpZM4UsqQS>
.
|
|
@rajbarik or @slavapestov It would be good to get a review on this, not just to unblock @rajbarik, but also to get bug fixes on 4.2 sooner. If you don't have time let me know so I can recruit someone for the review. |
|
@swift-ci test. |
|
Build failed |
|
Build failed |
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.
rajbarik
reviewed
Jun 25, 2018
| SILInstruction * | ||
| SILCombiner::createApplyWithConcreteType(FullApplySite Apply, | ||
| const ConcreteExistentialInfo &CEI, | ||
| SILBuilderContext &BuilderCtx) { |
There was a problem hiding this comment.
I'm limiting this PR to fixing correctness and trying hard to generalize the code without generalizing the current behavior.
The code in this function subsumes what used to be in multiple propagateConcreteTypeOfInitExistential functions (for both witness and protocol extension functions).
Relative to your PR #13991, it is one level above your simple createApplyWithConcreteType and could probably be reused by your propagateConcreteTypeOfInitExistentialToAllApplyArgs by generalizing the behavior. I think these PRs are mostly complementary and merging them later will be an additional code quality improvement.
rajbarik
reviewed
Jun 25, 2018
| // Create a set of arguments. | ||
| SmallVector<SILValue, 8> NewArgs; | ||
| for (auto Arg : Apply.getArgumentsWithoutSelf()) { | ||
| NewArgs.push_back(Arg); |
There was a problem hiding this comment.
Seems like only self arguments are optimized.
rajbarik
reviewed
Jun 25, 2018
|
|
||
| return NewAI; | ||
| return NewApply.getInstruction(); |
There was a problem hiding this comment.
At some point, we should merge what is there in PR #13991 to handle both self and non-self concrete type propagation. The concrete type can be obtained using a number of techniques including protocolconformance analysis, global_addr pattern match and init_existential pattern match. Makes sense?
There was a problem hiding this comment.
Yes, it should be fully generalized. I just need to separate out the bug fix which can hopefully be merged onto a release branch.
|
@slavapestov do you have time to review this for 4.2? |
|
@swift-ci test. |
|
@swift-ci test source compatibility. |
|
Build failed |
|
Build failed |
|
SCK fails on master: |
atrick
added a commit
that referenced
this pull request
Jun 27, 2018
* Reorganizing the code to find init_existential; Move them to Existential.cpp/h in order for other passes such as ExistentialSpecializer to use it apart from SILCombiner * [NFC] Add SILBuilderContext. 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) * Rewrite SILCombiner::propagateConcreteTypeOfInitExistential. (#17315) 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) * Merge rewrite SILCombiner::propagateConcreteTypeOfInitExistential onto 4.2.
rajbarik
reviewed
Jun 28, 2018
| InitExistential = findInitExistential(openedUse, OpenedArchetype, | ||
| OpenedArchetypeDef, isCopied); | ||
| if (!InitExistential) | ||
| return; |
There was a problem hiding this comment.
@atrick @slavapestov Could you decouple the body from the constructor and invoke it separately via an API? "findInitExistential" is not the only way to determine concrete types. For example, I am using ProtocolConformance Analysis to determine concrete types. I am planning on using the same ConcreteExistentialInfo struct and reuse the createApplyWithConcreteType. Let me know if you are busy and it sounds good to you, I can try to decouple them.
There was a problem hiding this comment.
@rajbarik All suggestions are welcome. I'm not dogmatic about how ConcreteExistentialInfo is constructed or accessed. I like to start with minimal design and develop outward. If you have a rough prototype, and want quick feedback, just point me to a branch or gist.
First thing to consider: do all the members of ConcreteExistentialInfo make sense for all the SIL patterns you're handling. It's fine to have a few optional fields:
- ConcreteTypeDef is null to when ConcreteType is not an opened archetype
- You could make InitExistential null to indicate the existential wasn't found.
Anything much beyond that and it may be better to have a discrimiated union or a smaller more general Info composing a larger one.
At any rate, I think it's fine if you optionally pass in a PCA to the ConcreteExistentialInfo construction, or even define a second constructor. If that's awkward for some reason you could move to a free standing or static function to create the Info.
There was a problem hiding this comment.
@atrick Thanks. I am currently adding a new constructor for CEI with values computed from PCA and this constructor will take a subset of the values currently present in the CEI. We can later think about a more tighter integration. However, one problem I am running into is regarding the following code in method createApplyWithConcreteType:
if (CEI.isCopied && !canReplaceCopiedSelf(Apply, CEI.InitExistential, DA))
When we use PCA, there is no InitExistential and no isCopied. So, the question is, can this check be moved to isValid() function in CEI or prepended with a check on InitExistential not being null? Do you have any recommendation?
There was a problem hiding this comment.
Please keep in mind that a check on InitExistential being not null, is minimal in terms of code changes :)
There was a problem hiding this comment.
The safe thing to do is to bail out under the isCopied case if we didn't find the init_existential.
if (isCopied && (!InitExistential || !canReplaceCopiedSelf(Apply, InitExistential)
I'm sure we could do better but if you don't care about that case and never hit it then why deal with it?
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.
Fixes rdar://40555427 [SR-7773]:
SILCombiner::propagateConcreteTypeOfInitExistential fails to full propagate type
substitutions.
This rewrite fixes several fundamental bugs in the SILCombiner optimization that
propagates concrete types. In particular, the pass needs to handle:
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.