-
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
[release/9.0] JIT ARM64: Don't emit mov
for zero case in jump tables for shift intrinsics
#107487
Conversation
This reverts commit cd2656f.
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
@tannergooding PTAL, thanks! |
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.
approved. please get a code review. we can merge when ready
@jeffschwMSFT I got approval from Tanner; could you please merge this when you have a moment? Thanks! |
@amanasifkhalid, you can merge after tests pass. |
@JulieLeeMSFT I'm afraid I don't have permission to merge into |
@JulieLeeMSFT could you please merge this when you get the chance? Thanks! |
cc @carlossanlop in case you're free |
Backport of #107322 to release/9.0
/cc @amanasifkhalid
Customer Impact
ARM64 has numerous shift intrinsics that encode the shift amount directly into the instruction. For right-shift intrinsics, we cannot directly encode a shift amount of zero, so another instruction must be used. Because shifting by zero is a no-op on its own, the JIT's emitter had a case to detect this pattern, and emit a
mov reg, reg
instruction instead (the details as to why we couldn't skip emitting an instruction altogether aren't wholly relevant here). This worked for intrinsics likeAdvSimd.ShiftRightLogical
, but for shift intrinsics with other side effects likeAdvSimd.ShiftLeftLogicalWideningLower
, this would drop the side effects when shifting by zero. Our fix is to remove this special case from the emitter, such that if we are shifting by zero, we simply emit the intrinsic to ensure its side effects are preserved (the emitter will already assert if we try to encode a zero shift amount into aShiftRight*
intrinsic).The importer already transforms uses of
ShiftRight*
intrinsics with zero shift amounts into fallback implementations, but we weren't covering code patterns likeVector128<byte> >> 8
, where the JIT masks the shift amount by the size of the base type such that it becomes zero. Thus, we've also updated the importer to convert such patterns to use a fallback implementation, too.This issue was discovered by Fuzzlyn, one of our test generation tools, and was tracked in #107173.
Regression
This seems to have been introduced during the .NET 9 dev cycle while fixing a different bug.
Testing
My PR includes regression tests to cover various intrinsic patterns that were explicitly fixed. This PR also passed our current suite of HW intrinsic tests, which exercise every
AdvSimd.Shift*
intrinsic (as well as the<<
and>>
operators) with various inputs.Risk
Low. This fix changes codegen for a small subset of
AdvSimd
intrinsics when shifting by zero, as well as the>>
operator when shifting by multiples of the base type's size. I imagine neither of these patterns is all that common in our users' code.