-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enregister EH var that are single def #47307
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
015a7ce
Enable EhWriteThry for SingleDef
kunalspathak 71b5c52
If EhWriteThru is enabled, DoNotEnregister if variable is not singleDef
kunalspathak ca39359
Revert code in ExecutionContext.RunInternal
kunalspathak f51bc7c
Revert code in AsyncMethodBuildCore.Start()
kunalspathak e434daa
Make sure we do not reset lvSingleDef
kunalspathak a85efd2
Consitent display of frame offset
kunalspathak aff80de
Use lvEHWriteThruCandidate
kunalspathak 24b0af6
Do not enregister EH Var that has single use
kunalspathak ba57957
do not enregister simdtype
kunalspathak 061f8e0
add missing comments
kunalspathak 802424a
jit format
kunalspathak c28e628
revert an unintended change
kunalspathak fc6823e
jit format
kunalspathak a3a9967
Add missing comments
kunalspathak File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand this -- if we dead store something we can turn it from multiple def to single def. I suppose that won't happen for live into handler vars but it can for other vars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not 100% sure if that is the case.
Currently, in the PR, I am marking
lvEhWriteThruCandidate
in one of the early phase oflvaMarkLocalVars()
. I have to do it because that information is used during SSABuilder/Lowering to determine if avarDsc
should be enregistered or not. The weird part of this logic forlvaSetVarLiveInOutOfHandler()
is that during SSABuilder, we might mark avarDsc->lvDoNotEnregister=1
but later during Lowering, if things change and we want to resetlvDoNotEnregister
to enregister that EH variable, currently we don't do that. That should be fixed IMO.As per your suggestion, I should definitely take into account dead store, etc. and defer the setting of
lvEhWriteThruCandidate
until I recompute the ref count during Lowering, but it would be too late and we might have already marked thevarDsc
to be "do not enregister".So perhaps, there might be some refactoring involved to accomplish things to happen in right order and I might want to do it in a separate PR. Other thing that I want to do in the follow-up PR is to support local fields, because those are skipped in this PR from getting marked as
lvEhWriteThruCandidate
. Perhaps, the logic of settinglvEhWriteThruCandidate
should ideally be around this code.