Skip to content
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

Merged
merged 11 commits into from
Sep 11, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 6, 2024

Backport of #107322 to release/9.0

/cc @amanasifkhalid

Customer Impact

  • Customer reported
  • Found internally

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 like AdvSimd.ShiftRightLogical, but for shift intrinsics with other side effects like AdvSimd.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 a ShiftRight* intrinsic).

The importer already transforms uses of ShiftRight* intrinsics with zero shift amounts into fallback implementations, but we weren't covering code patterns like Vector128<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

  • Yes
  • No

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.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 6, 2024
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@amanasifkhalid
Copy link
Member

@tannergooding PTAL, thanks!

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a 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 jeffschwMSFT added the Servicing-approved Approved for servicing release label Sep 9, 2024
@amanasifkhalid
Copy link
Member

@jeffschwMSFT I got approval from Tanner; could you please merge this when you have a moment? Thanks!

@JulieLeeMSFT
Copy link
Member

@amanasifkhalid, you can merge after tests pass.

@amanasifkhalid
Copy link
Member

@JulieLeeMSFT I'm afraid I don't have permission to merge into release/9.0.

@amanasifkhalid
Copy link
Member

@JulieLeeMSFT could you please merge this when you get the chance? Thanks!

@amanasifkhalid
Copy link
Member

@JulieLeeMSFT could you please merge this when you get the chance? Thanks!

cc @carlossanlop in case you're free

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants