ARCSequenceOpts: Add LoopSummary verifier #33810
Merged
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 |
|
@swift-ci benchmark |
|
@swift-ci Please Test Source Compatibility |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibsHow 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
Sep 11, 2020
| #ifndef NDEBUG | ||
| // Verify updateForDifferentLoopInst is conservative enough that the flow | ||
| // sensitive native of the loop summarized instructions does not matter. | ||
| if (verifyARCLoopSummary) { |
There was a problem hiding this comment.
Is this verification inside the loop over SummarizedInterestingInsts? Shouldn't it be outside? Otherwise it's unnecessarily quadratic
There was a problem hiding this comment.
Thanks. I fixed it.
meg-gupta
force-pushed
the
verifyloopsummary
branch
from
0db4137 to
e1cc16e
Compare
|
@swift-ci test |
ARCSequenceOpts with loop support works on regions inside out. While processing an outer region, it uses summary from the inner loop region to compute RefCountState transitions used for CodeMotionSafe optimization. We do not compute the loop summary by iterating over it twice or iterating until a fixed point is reached. Instead loop summary is just a list of refcount effecting instructions. And BottomUpRefCountState::updateForDifferentLoopInst and TopDownRefCountState::updateForDifferentLoopInst are used to compute the RefCountState transition when processing the inner loop region. These functions are very conservative and the flow sensitive nature of the summarized instructions do no matter. This PR adds a verifying assert to confirm this.
meg-gupta
force-pushed
the
verifyloopsummary
branch
from
e1cc16e to
ee19299
Compare
atrick
approved these changes
Sep 11, 2020
|
@swift-ci test |
|
@swift-ci Please Test Source Compatibility |
|
Build failed |
|
Build failed |
|
Errors in sck are all known SILGen bugs. |
meg-gupta
force-pushed
the
verifyloopsummary
branch
from
ee19299 to
908ad5d
Compare
|
Removed commit for testing |
|
@swift-ci smoke test |
|
@swift-ci smoke test Linux platform |
|
@swift-ci smoke test OSX platform |
|
@swift-ci smoke test OS X platform |
1 similar comment
|
@swift-ci smoke test OS X platform |
dabrahams
pushed a commit
that referenced
this pull request
Sep 15, 2020
* 'master' of github.com:apple/swift: [docs] Fix broken links in the documentation index (#33829) [Profiler] Increment closure body count prior to the prolog (#33946) ARCSequenceOpts: Add LoopSummary verifier (#33810) [build-script] Cleanup source directory layout help Address review comment. ABI checker: when invoking via build system, explicitly mention ABI breakge in diagnostics stdlib: Remove unused, unsafe helper function _withUninitializedString (#33704) test: Replace _silgen_name w/ _cdecl in CommandLineStressTest [AutoDiff] [Docs] Clarify 'Differentiable' derived conformances conditions. Revert "build-script: remove dead CMake options for Swift"
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.
ARCSequenceOpts with loop support works on regions inside out. While processing an outer region, it uses summary from
the inner loop region to compute RefCountState transitions used for CodeMotionSafe optimization.
We do not compute the loop summary by iterating over it twice or iterating until a fixed point is reached.
Instead loop summary is just a list of refcount affecting instructions. And BottomUpRefCountState::updateForDifferentLoopInst and TopDownRefCountState::updateForDifferentLoopInst are used to compute the RefCountState transition when processing the inner loop region. These functions are very conservative and the flow sensitive nature of the summarized instructions do no matter.
This PR adds a verifying assert to confirm this.