Uh oh!
There was an error while loading. Please reload this page.
[release/10.0] Fix SIMD MinMax constant special cases - #133257
Open
tannergooding wants to merge 1 commit into
Open
[release/10.0] Fix SIMD MinMax constant special cases#133257tannergooding wants to merge 1 commit into
tannergooding wants to merge 1 commit into
Conversation
SIMD Min/Max constant handling used whole-vector checks for NaN and signed zero when deciding whether native xarch Min/Max needed compensation. Constants containing a mix of ordinary and special lanes could therefore lose the managed NaN or signed-zero semantics. This evaluates the predicates through reusable SIMD mask helpers matching `IsNaN`/`IsNegative` and `AnyWhereAllBitsSet`/`AllWhereAllBitsSet`, then uses any-lane checks for fast-path eligibility. Fixesdotnet#133022 > [!NOTE] > This pull request was created with GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 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
🔵 Needs a closer look
The changes affect CoreCLR JIT SIMD min/max code generation and AVX-512 fixup behavior, which is correctness-sensitive and warrants final expert/hardware validation beyond automated review.
Review tier: Lite
Findings: None
What changed in this PR
Backport to release/10.0 of the SIMD Min/Max constant-lane fix, addressing incorrect xarch results when a constant vector mixes ordinary lanes with NaN and/or signed-zero lanes (notably in the AVX-512 path that could incorrectly return the opaque operand wholesale).
Changes:
- Updates CoreCLR JIT SIMD constant classification and per-lane fixup-table construction to correctly handle mixed NaN and signed-zero lanes.
- Adds a focused JIT regression test project (
Runtime_133022) covering mixed-lane NaN and signed-zero behavior acrossMin/Max/MinNumber/MaxNumber. - Extends generic math test member data with NaN payload + signed-zero edge cases to validate correct IEEE/managed semantics.
| File | Description |
|---|---|
| src/coreclr/jit/simd.h | Extends SIMD constant-evaluation helpers with simdSize and adds reusable “mask” predicates (NaN/negative/zero/signed-zero) plus any/all checks. |
| src/coreclr/jit/gentree.h | Adds GenTreeVecCon helpers to detect NaN / negative-zero / positive-zero presence in vector constants. |
| src/coreclr/jit/gentree.cpp | Switches vector-constant special-case checks to the new per-lane helpers and refines AVX-512 fixup-table construction for mixed signed-zero lanes. |
| src/tests/JIT/Regression/JitBlue/Runtime_133022/Runtime_133022.csproj | Introduces a standalone JIT regression project for the mixed-lane SIMD Min/Max issue. |
| src/tests/JIT/Regression/JitBlue/Runtime_133022/Runtime_133022.cs | Adds regression coverage for mixed NaN / signed-zero lanes across Vector128/Vector256 and float/double. |
| src/libraries/Common/tests/System/GenericMathTestMemberData.cs | Adds NaN-payload + signed-zero test vectors for Min/Max/MinNumber/MaxNumber scalar semantics validation. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
AndyAyersMS
approved these changes
Sep 4, 2026
This was referenced Sep 5, 2026
Open
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 freeto 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.
Backport of #133173 to release/10.0
/cc @AndyAyersMS
Customer Impact
SIMD
Min,Max,MinNumber, andMaxNumbercan return incorrect results on xarch when a constant vector contains a mix of ordinary values and NaN or signed-zero lanes. The AVX-512 path can incorrectly return the opaque operand wholesale.Regression
The vector mixed-lane regression was introduced by #116804 during .NET 10 development.
Testing
The focused
Runtime_133022regression fails against the pre-fix JIT and passes with the fix. The original change was verified on checked x64 and x86 with AVX-512 enabled and withDOTNET_EnableAVX512=0; all 13,082System.Runtime.Intrinsicstests also passed under both AVX-512 settings.For this backport, the checked x64 JIT build and
jit-formatpassed. The product-code conflict was resolved to match the merged mixed-negative-zero handling, andRuntime_133022was adapted to this branch's standalone regression-project layout. A fullclr+libsbaseline was attempted but hit unrelated MSVCC1041PDB contention inSystem.Globalization.Native.Risk
Medium. The change is localized to JIT import-time SIMD constant classification and fixup-table construction, but affects shared Min/Max code generation. Mixed NaN and signed-zero cases are covered across
float/double,Vector128/Vector256, operand order, and AVX-512 enabled/disabled configurations.IMPORTANT: If this backport is for a servicing release, please verify that:
release/X.0-staging, notrelease/X.0.release/X.0(no-stagingsuffix).Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.
Note
This pull request description was generated with GitHub Copilot assistance.