-
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
Fixes #106142. #106369
Fixes #106142. #106369
Conversation
9115a8a
to
9d764fa
Compare
@tannergooding @jakobbotsch , please review the code before RC1 snap. |
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.
Can you add the test?
Thanks for the work. If tests pass, I will merge today. |
@@ -802,17 +802,27 @@ TBase EvaluateBinaryScalarSpecialized(genTreeOps oper, TBase arg0, TBase arg1) | |||
case GT_ROL: | |||
{ | |||
// Normalize the "rotate by" value | |||
arg1 %= (sizeof(TBase) * BITS_PER_BYTE); |
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.
Was the behavioral difference for negative inputs, since %=
is remainder of truncating division and so handles signed values differently?
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.
Yep. It was different for negative inputs. For eg. one of the test case has the rotate count show up as -2 in here.
This Fixes #106142
Rotate requires the count to be normalized depending on base type. This was already done with f251b17 but there was a behavioral mismatch between x86 and x64 with the mod(%) operator