-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Constrained mask should take into account busy registers #85933
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsWhen we constrained the registers under JitStressRegs, check if the constrained masks is not left with "busy registers" because in that case, we will not be able to allocate register. Relax that and do not constraint if they coincides with "busy registers". Fixes: #85627
|
I am seeing superpmi-replay failures similar to one reported in #59916. |
@dotnet/jit-contrib |
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.
Mostly nits but one question
src/coreclr/jit/codegenxarch.cpp
Outdated
lclNum2 = op2Skip->AsLclVarCommon()->GetLclNum(); | ||
} | ||
#endif | ||
noway_assert(GenTree::OperIsCommutative(oper) || (lclNum1 == lclNum2)); |
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 realized this can't be a noway_assert
now: it would mean we're asserting different things in Release and Debug: and the case where you're asserting lclNum1 == lclNum2
would fail in Release.
When we constrained the registers under JitStressRegs, check if the constrained masks is not left with "busy registers" because in that case, we will not be able to allocate register. Relax that and do not constraint if they coincides with "busy registers".
Fixes: #85627
(Most likely) fixes: #76382
Also noticed a problem where we were not taking into account
SUB_HI
andLSH_HI
for RMW analysis. Doing that discovered that we should update the assert about Commutative() to take into account if both operands are same. Both of these will fix the following issue:Fixes: #59916