-
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
JIT: Account for GT_JMP implicit uses in local morph ref counting #80734
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak |
cc @dotnet/jit-contrib PTAL @AndyAyersMS |
Let me see if I can do something for the MinOpts TP regression. |
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.
Ah, I see normal ref counting has a special clause for register args when jmp is used.
src/coreclr/jit/lclmorph.cpp
Outdated
case GT_LCL_FLD: | ||
MorphLocalField(node, user); | ||
assert(node->OperIsLocal()); | ||
__fallthrough; |
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.
__fallthrough; | |
FALLTHROUGH; |
(We use the macro version everywhere else)
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 ended up reverting this because the case above also can potentially morph to a local, so also needs some more handling. There is a BasicBlock helper already to check for GT_JMP
that it uses now.
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.
Just for my understanding: why not use compJmpOpUsed
?
(Multiple JMP
s?)
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.
Yes (BasicBlock::endsWithJmpMethod
does use compJmpOpUsed
as an early-out)
One diff in the test that exposed the problem -- we no longer incorrectly omit a copy there. |
Fix #80731
Found by #80691.