SILOptimizer: Add a new TempRValue optimization pass [4.0] #11364
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
This is a separate optimization that detects short-lived temporaries that can be eliminated. This is necessary now that SILGen no longer performs basic RValue forwarding in some cases. SR-5508: Performance regression in benchmarks caused by removing SILGen peephole for LoadExpr in +0 context
|
@swift-ci Please test |
|
This shouldn't hold up the 4.0 fix, but: you say that in some cases we end up copying the whole struct value when it was not copied in the original source. Is it possible that the LoadExpr is simply being applied at too coarse a level, i.e. that the type-checker is producing Project(Load(x)) instead of Load(Project(x))? Because we may be able to fix that in a fairly direct way without breaking ownership rules, either by changing CSApply or by changing SILGen to peephole that. |
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.
Description: This is a separate optimization that detects short-lived temporaries that can be eliminated. This is necessary now that SILGen no longer performs basic RValue forwarding in some cases.
Scope of the issue: Originally we thought it's just a 30% performance regression for a certain type of micro benchmarks. But the problem is worse. It can happen that in a mutating struct method the whole struct is copied, even if the copy is not done in the source code. So this performance regression can be significant.
Origination: The performance regression was introduced with a SILGen bug fix: #11026
Tested: By lnt tests
Risk: Low. Although the patch adds quite a few lines of codes, I think the risk is low, because:
-) The logic of the new optimization pass is easy to understand, so there should be no conceptual problems.
-) 3 people reviewed the change
-) It's an isolated optimization pass which just runs once at the beginning of the optimization pipeline. So it should not see any unusual code generated by other optimizations. Also, it does not interfere with the internal state of another optimization (like CopyForwarding, which was the case in the first implementation).
-) I added many test cases which also test all the negative scenarios (where the optimization should bail).
-) If it still turns out that there is a problem, this optimization pass can be easily disabled with a command line option
Reviewed by: @atrick, @gottesmm and @swiftix
Jira/Radar: SR-5508, rdar://problem/33404490