SILOptimizer: fix a miscompile in TempRValueOpt and improve MemBehavior #34252
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
The InspectionMode was never set to anything else than "IgnoreRetains"
1. Do a better alias analysis for "function-local" objects, like alloc_stack and inout parameters 2. Fully support try_apply and begin/end/abort_apply So far we fully relied on escape analysis. But escape analysis has some shortcomings with SIL address-types. Therefore, handle two common cases, alloc_stack and inout parameters, with alias analysis. This gives better results. The biggest change here is to do a quick check if the address escapes via an address_to_pointer instructions.
…rce in a called functions correctly. This fixes a miscompile in case the source of the optimized copy_addr is modified in a called function with to a not visible alias. This can happen with class properties or global variables. This fix removes the special handling of function parameters, which was just wrong. Instead it simply uses the alias analysis API to check for modifications of the source object. The fix makes TempRValueElimination more conservative and this can cause some performance regressions, but this is unavoidable. rdar://problem/69605657
…ine. This can compensate the performance regression of the more conservative handling of function calls in TempRValueOpt (see previous commit). The pass runs after the inlining passes and can therefore optimize in some cases where it's not possible before inlining.
Contributor
Author
|
@swift-ci test |
Contributor
Author
|
@swift-ci benchmark |
Contributor
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -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 |
atrick
reviewed
Oct 12, 2020
Contributor
atrick
left a comment
atrick
left a comment
There was a problem hiding this comment.
Out of curiosity, what do you mean by "the regressions are unavoidable"? Do you mean that the regressions are all cases where we can't possibly determine that the source of the copy is not written within the callee function? Is it because the source is a class property and the class reference was passed in as an argument to the current function?
I have two requests for change below, and one response to your code comment...
| // for these. | ||
| // | ||
| // FIXME: Once we separate the notion of ref counts from reading/writing | ||
| // memory this will be unnecessary. |
Contributor
There was a problem hiding this comment.
Is this FIXME supposed to go away now?
| // For exclusive/local addresses we can do a quick and good check with alias | ||
| // analysis. For everything else we use escape analysis (see below). | ||
| // TODO: The check for not-escaping can probably done easier with the upcoming | ||
| // API of AccessStorage. |
Contributor
There was a problem hiding this comment.
Thanks. Yes, AccessedStorage replaces getUnderlyingObject and my current branch has a "use visitor" that hasEscapingUses will trivially plug into. This is exactly how I expected the new API to be used, and this is specifically one of the things I wanted to do to improve alias analysis. The new utilities will remove remove some of the implementation here, ensure that the def-use/use-def logic is symmetric, complete and consistent with all the other address analysis in the compiler, and independently verified/tested.
| // reference argument. In this case V clearly "aliases" the argument, but | ||
| // this is not reported by alias analysis. | ||
| if ((!nonEscapingAddress && !arg->getType().isAddress()) || | ||
| mayAlias(arg)) { |
Contributor
There was a problem hiding this comment.
The previous code was carefully designed to avoid calling alias analysis unless the argument behavior changed. I think you should change it back.
Contributor
Author
There was a problem hiding this comment.
I did see the intention of the original code.
I did some experiments (for another reason) to check how often this code is actually called and found that it's not called very heavily.
Therefore I decided to "optimize" for clarity.
Contributor
Author
|
About the regressions: some regressions are noise and (I believe) some regressions are due to inlining decisions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Why include an optimization improvement and a correctness fix in one PR?
Unfortunately the correctness fix makes TempRValueElimination more conservative and this can cause some performance regressions. To compensate for this, I improved the handling of apply instructions in MemBehavior.
This change fixes a miscompile in case the source of the optimized copy_addr is modified in a called function via an invisible alias. This can happen with class properties or global variables.
This fix removes the special handling of function parameters, which was just wrong.
Instead it simply uses the alias analysis API to check for modifications of the source object.
The changes to improve MemBehavior for apply instructions include:
For details, see the commit messages.
rdar://problem/69605657