[semantic-arc-opts] Implement @owned phi web elimination for phi webs with a single phi node that only have copy_value introducers. #30289
Conversation
This file contains 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 test source compatibility |
|
This is also one of the most simple cases, so I am not necessarily expecting a large change in perf. Will take what I can get though = p. |
Performance: -OCode size: -OPerformance: -Osize
Code size: -OsizePerformance: -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 |
|
I think the release failure is a problem on the bot. Saw this: |
|
Reproduced the failure locally. I wonder if it is non-deterministic. |
|
Debug source compat passed. I think the release one hit a race. Going to rerun. |
|
@swift-ci test source compatibility |
|
One of the failures in release suite looks real and actually related to the logic. Other has to do with my not being aggressive enough using autogenerated locations. |
|
Looking at the failures now. |
… with a single phi node that only have copy_value introducers. This is the most simple initial version that I can commit. The hope is that this will help to bring this up in a nice way. I am going to handle the multiple phi node and load [copy] case later to reduce code churn. <rdar://problem/56720436>
|
Found the core store issue. I used a propagated a branch location onto a begin_borrow. I just changed it to use an autogenerated location. |
gottesmm
force-pushed
the
pr-645bd01cfa67ffe64ed23d74e1ee1333fce6ba8a
branch
from
0245ac3 to
6fee59b
Compare
|
Both turned out to be the loc issue. |
|
@swift-ci test source compatibility |
|
@swift-ci test |
|
@atrick was just thinking. I think the same approach here would work with tuples and other aggregate forming instructions. They act as a phi in a certain sense of their operand's ownership. |
|
I talked with Andy offline. I am going to refactor parts of it according to his asks in a forthcoming commit. |
atrick
reviewed
Mar 10, 2020
| return destroyingUses.front(); | ||
| } | ||
|
|
||
| /// If this LiveRange has a single unknown destroying use, return that |
There was a problem hiding this comment.
unknown destroying use
To be consistent: "unknown consuming use"
| ArrayRef<Operand *> getNonConsumingForwardingUses() const { | ||
| return generalForwardingUses; | ||
| } | ||
|
|
||
| void convertOwnedGeneralForwardingUsesToGuaranteed(); |
There was a problem hiding this comment.
convertOwnedGeneralForwardingUsesToGuaranteed
This declaration has a cryptic name, so should have a doc comment description
There was a problem hiding this comment.
I have a patch but it conflicts my other fix patch. I am going to wait for that to land then get to this.
|
Hi @gottesmm – This breaks my Fedora 31 box (x86_64). Do you have a quick fix or can we revert this? [EDIT] – Updated with a debug backtrace: |
gottesmm
added a commit
that referenced
this pull request
Mar 11, 2020
…24a8c8f9422956 Recommit #30289 with ASAN fix.
gottesmm
added a commit
that referenced
this pull request
Mar 11, 2020
…0070d8cd472328 [semantic-arc-opts] Follow ups to #30289
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.
This is the most simple initial version that I can commit. The hope is that this will help to bring this up in a nice way.
I am going to handle the multiple phi node and load [copy] case later to reduce
code churn.
rdar://problem/56720436