[PassManager] Update PassManager's function worklist for newly added SILFunctions #30710
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
|
@swift-ci test compiler performance |
|
@swift-ci please benchmark |
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 |
|
@meg-gupta can you give a more descriptive commit message? I think the title of the PR would be fine for the commit title and I would add to the commit msg the general effort you are working on. |
meg-gupta
force-pushed
the
bottomupfunction
branch
from
c5deb51 to
76af58b
Compare
meg-gupta
changed the title
|
@meg-gupta Nice! That is a really great commit message! = ). |
meg-gupta
changed the title
eeckstein
requested changes
Mar 31, 2020
| void SILPassManager::recursivelyUpdateFunctionWorklist( | ||
| SILFunction *F, SmallPtrSetImpl<SILFunction *> &Visited) { | ||
| BasicCalleeAnalysis *BCA = getAnalysis<BasicCalleeAnalysis>(); | ||
| auto findFunc = [this](SILFunction *Func) { |
There was a problem hiding this comment.
This linear search results in quadratic complexity. The FunctionWorkList can contain thousands of entries.
The same with FunctionWorklist.erase(CalleeIt).
What you could do is to use BottomUpFunctionOrder to get the list of functions which need to be reordered at the end of FunctionWorkList (see my previous comment).
Then in a single sweep reorder FunctionWorkList - depending if a function is in BottomUpFunctionOrder result list or not.
This can still be quadratic in case there are a lot of new functions added (but at least a single reorder is linear). To solve this problem, maybe it's sufficient to reorder the FunctionWorkList not every time a new function is added, but only if a correctly sorted list is needed: and that's mainly before the inliner runs.
There was a problem hiding this comment.
Am worried that it is not just the inliner which needs it in bottom up. ArrayPropertyOpt also computes profitability of callees. I have to look how many more function passes will look at callee function bodies.
|
@meg-gupta I don't care about benchmark code size at all, but before you commit can you check how bad the SCK code size regressions are? |
|
@atrick I see only a minimal change. This list just consists of macos SCK: |
|
@swift-ci please benchmark |
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 |
eeckstein
requested changes
Apr 3, 2020
eeckstein
approved these changes
Apr 6, 2020
|
@eeckstein I did not investigate each of the regressions. But the regressions from the benchmarks in |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
The PassManager should transform all functions in bottom up order. This is necessary because when optimizations like inlining looks at the callee function bodies to compute profitability, the callee functions should have already undergone optimizations to get better profitability estimates. The PassManager builds its function worklist based on bottom up order on initialization. However, newly created SILFunctions due to specialization etc, are simply appended to the function worklist. This can cause us to make bad inlining decisions due to inaccurate profitability estimates. This change now updates the function worklist such that, all the callees of the newly added SILFunction are proccessed before it by the PassManager. Fixes rdar://52202680
meg-gupta
force-pushed
the
bottomupfunction
branch
from
8263397 to
fd98ce1
Compare
|
@swift-ci please test |
|
@swift-ci please benchmark |
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 |
|
@swift-ci please test |
|
@swift-ci Please Test Source Compatibility |
|
@swift-ci benchmark |
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 |
|
@swift-ci Please Test Source Compatibility |
|
@swift-ci Please Test Source Compatibility Debug |
|
@swift-ci test |
texasmichelle
added a commit
that referenced
this pull request
Nov 3, 2020
Reverts the reversion in #34188. Bisection points to #34188 as the cause of a crasher that looks to be the result of a miscompile. Adding this back resolves the crasher. Details of the crash: The `DiscontiguousSliceSlicing` test in validation-test/StdlibUnittest/RangeSet.swift crashes in the `every()` function on line 15. When the contents of this function are replaced with a functional equivalent that uses different syntax, the crash resolves.
ainu-bot
added a commit
to google/swift
that referenced
this pull request
Nov 3, 2020
…tage * 'tensorflow' of github.com:apple/swift: Re-add swiftlang#30710 Set SWIFTCI_USE_LOCAL_DEPS=1 update-checkout-config.json: adjust for tensorflow merge
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.
The PassManager should transform all functions in bottom up order.
This is necessary because when optimizations like inlining looks at the
callee function bodies to compute profitability, the callee functions
should have already undergone optimizations to get better profitability
estimates.
The PassManager builds its function worklist based on bottom up order
on initialization. However, newly created SILFunctions due to
specialization etc, are simply appended to the function worklist. This
can cause us to make bad inlining decisions due to inaccurate
profitability estimates. This change now updates the function worklist such
that, all the callees of the newly added SILFunction are proccessed
before it by the PassManager.
Fixes rdar://52202680