Enable mem2reg of enum types #65085
Merged
Enable mem2reg of enum types #65085
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
| if (asi->getType().isOrHasEnum()) { | ||
| for (auto it : initializationPoints) { | ||
| auto *si = it.second; | ||
| auto src = si->getOperand(0); |
There was a problem hiding this comment.
Should [a function extracted from--no need to find the block, only to find the appropriate value to use] StackAllocationPromoter::getLiveOutValues(_, it.first) be used here to find the lexical borrows or moves that may have been introduced?
There was a problem hiding this comment.
Lifetime completion will work on the inner borrows. But looks like it doesn't for inner moves (or any other forwarding consumes). Also lifetime completion of lexical values (move_value [lexical]) is only possible at the unreachable blocks, what we really need is lifetime completion at the boundary in this case in mem2reg. Looks like some additional work is needed here..
There was a problem hiding this comment.
Going to wait for @atrick's comment here about the lifetime completion utility's use in this case.
@nate-chandler Until then, I have disabled this for lexical stack locs.
There was a problem hiding this comment.
@meg-gupta @nate-chandler For the record...
Lifetime completion only handles inner scopes that might affect the lifetime of the outer scope. It doesn't make sense for lifetime completion to look past any consumes.
This will, however, change when we fully support a mark_dependence where an owned value depends on a borrow scope. To handle that, we'll need to look through all potentially forwarding consumes. The final destroy will need to be within the borrow scope. That change only affects cases where such a mark_dependence exists
meg-gupta
force-pushed
the
mem2regenableoptional
branch
from
f7b443f to
70069f9
Compare
|
@swift-ci test |
|
Needs #65074 to be merged |
nate-chandler
approved these changes
Apr 12, 2023
|
@swift-ci test |
|
@swift-ci please benchmark |
|
@swift-ci Please Apple Silicon benchmark |
Previously it was disabled because we have incomplete address lifetimes for such types, and transforming to value form in mem2reg would expose verification errors due to incompleteness. Enable this case here and fixup the lifetimes using the new lifetime completion utility.
meg-gupta
force-pushed
the
mem2regenableoptional
branch
from
70069f9 to
96a6d44
Compare
|
@swift-ci smoke test and merge |
|
I think that this might be causing problems on the Windows builds: |
|
CC: @MaxDesiatov |
|
@swift-ci please build toolchain Windows platform |
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.
Previously it was disabled because we have incomplete address lifetimes for such types,
and transforming to value form in mem2reg would expose verification errors due to incompleteness.
Enable this case here and fixup the lifetimes using the new lifetime completion utility.