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.
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
Enregister EH var that are single def #47307
Changes from 13 commits
015a7ce
71b5c52
ca39359
f51bc7c
e434daa
a85efd2
aff80de
24b0af6
ba57957
061f8e0
802424a
c28e628
fc6823e
a3a9967
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Maybe add comment on why these cause exclusion?
USEASG
I can guess, at though it seems it would be redundant with restricting to single def?COLON_COND
is a mystery to me, why does it matter? It goes away during morph.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.
To be honest, I just copied the conditions of
USEASG
andCOLON_COND
fromlvSingleDef
logic above. Yes, I don't think we need it specially because we already check forvarDsc->lvEhWriteThruCandidate
.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.