Conversation
|
Azure Pipelines: Successfully started running 7 pipeline(s). 9 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Two moderate findings remain unresolved in type matching and test independence.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
Adds JIT recognition and code generation for complementary constant funnel shifts using SHRD on x64 and EXTR on ARM64.
Changes:
- Adds matching, lowering, containment, and register-allocation support.
- Adds architecture-specific instruction generation.
- Adds correctness, boundary, aliasing, randomized, and negative-pattern tests.
| File | Summary |
|---|---|
src/tests/JIT/opt/Add/FunnelShift.csproj |
Configures the funnel-shift tests. |
src/tests/JIT/opt/Add/FunnelShift.cs |
Tests correctness and generated instructions. Moderate finding (2 votes): Overlap and Gap need an independent oracle or explicit negative disassembly checks. |
src/coreclr/jit/lsraxarch.cpp |
Adds x64 register constraints. |
src/coreclr/jit/lowerxarch.cpp |
Integrates x64 containment. |
src/coreclr/jit/lowerarmarch.cpp |
Integrates ARM64 containment. |
src/coreclr/jit/lower.h |
Declares funnel-shift lowering helpers. |
src/coreclr/jit/lower.cpp |
Matches and contains funnel-shift patterns. Moderate finding (1 vote): the type guard excludes TYP_ULONG and TYP_UINT; the related GenTree::IsFunnelShift assertion also needs updating. |
src/coreclr/jit/gentree.h |
Declares funnel-shift shape validation. |
src/coreclr/jit/gentree.cpp |
Implements funnel-shift shape validation. |
src/coreclr/jit/codegenxarch.cpp |
Emits SHRD. |
src/coreclr/jit/codegenarm64.cpp |
Emits EXTR. |
Comment on lines
+133
to
+134
| Overlap(lo, hi) != ((lo >> 17) | (hi << 46)) || | ||
| Gap(lo, hi) != ((lo >> 17) | (hi << 48)) || |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Recognize
(lo >> count) | (hi << (width - count))with complementary constant counts and emit SHRD on x64 or EXTR on ARM64. Keep the original memory-access ordering and reject shapes that cannot safely share the two input registers.Split from #134039 as suggested by @tannergooding. This is independent of the carry/borrow and multiply-accumulate changes in that PR.
Includes containment, register-allocation and code-generation support, plus boundary, randomized, aliasing and negative-pattern tests.
Validation:
git diff --checkpassed. Native ARM64 execution and a full SuperPMI sweep were not run for this extraction.