Skip to content

Conversation

@csa7mdm
Copy link

@csa7mdm csa7mdm commented Dec 14, 2025

Description

This PR addresses issue #74020 by implementing a peephole optimization in JIT Lowering to combine consecutive right shift operations (GT_RSH/GT_RSZ) with constant shift amounts.

Fix

The optimization logic is added to Lowering::LowerShift and handles:

  • Nested shifts of the same operation type: (x >> c1) >> c2 -> x >> (c1 + c2)
  • Nested shifts separated by a cast (common in division lowering): (cast (x >> c1)) >> c2 -> cast (x >> (c1 + c2))

Verification

  • Verified against the scenario reported in the issue where repeated division by constants generates redundant shift instructions.
  • Ensure combined shift count does not exceed type width.

Ahmed Mustafa added 2 commits December 9, 2025 19:52
This change implements a peephole optimization in Lowering::LowerShift to combine consecutive right shifts (RSH/RSZ) with constant amounts. It specifically addresses cases where division optimization introduces redundant shifts, such as (x / c1) / c2.
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 14, 2025
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Dec 14, 2025
@csa7mdm csa7mdm force-pushed the fix/74020-jit-shift-opt branch from bd15d1d to 4aa8c26 Compare December 15, 2025 11:50
@csa7mdm csa7mdm force-pushed the fix/74020-jit-shift-opt branch from 0b10860 to cca8115 Compare December 15, 2025 12:43
@vcsjones vcsjones added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Dec 17, 2025
Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I do still think this would be better to handle in morph and that it would be good to track RSH(x, n) for 0 < n < bitWidth as being "never negative", but will defer to other input.

CC. @dotnet/jit-contrib for secondary review

@JulieLeeMSFT
Copy link
Member

LGTM. I do still think this would be better to handle in morph and that it would be good to track RSH(x, n) for 0 < n < bitWidth as being "never negative", but will defer to other input.

@csa7mdm, I also think it would be better to handle it in morph. Do you want to try that?

@JulieLeeMSFT JulieLeeMSFT added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 26, 2026
@dotnet-policy-service dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 26, 2026
@csa7mdm
Copy link
Author

csa7mdm commented Jan 27, 2026

n workflow(s) awaiting approva

@csa7mdm
Copy link
Author

csa7mdm commented Jan 27, 2026

n workflow(s) awaiting approval

@JulieLeeMSFT
Copy link
Member

Approved workflow to run.

Ahmed Mustafa and others added 2 commits January 27, 2026 02:50
This file was unintentionally modified in the PR. The change added ARM
architecture to the BuildDebPackage and BuildRpmPackage conditions,
which is unrelated to the JIT shift optimization work and causes test
infrastructure failures by affecting how test packages are built.

Reverting to match the upstream main branch to fix Helix test failures.
@adamperlin
Copy link
Contributor

@csa7mdm it looks like the latest CI run failed if you want to take a look?

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 community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants