Fix two latent HWIntrinsic miscompiles in gentree.cpp#130832
Open
tannergooding wants to merge 6 commits into
Open
Fix two latent HWIntrinsic miscompiles in gentree.cpp#130832tannergooding wants to merge 6 commits into
tannergooding wants to merge 6 commits into
Conversation
The scalar-only IsScalarZero refinement guarded on the isScalar pointer (always non-null) rather than *isScalar, so a packed SUB-mapped intrinsic with a constant op1 whose low element is zero -- but which is not all-zero, e.g. Vector128.Create(0, 5, 6, 7) - x -- was mis-mapped to GT_NEG. morph consumes the packed effective oper and drops the real constant, a latent miscompile. Dereference the flag so the refinement only applies to scalar ops. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The min-branch signed-zero fast path computed eedsFixup per isNumber/isScalar, then an unconditional trailing block overwrote it with IsVectorNegativeZero for all four cases -- so scalar and non-negative-zero vector cases got the wrong value. When it wrongly lands alse, the const-fold fast path is taken without the required AVX512 fixup, a latent miscompile of Min with a signed-zero constant. Mirror the correct max branch: the non-scalar non-number case uses IsVectorNegativeZero and the stray block is removed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Covers packed integer `(cns - v1) + v2` where the constant's low lane is zero, which was previously miscompiled by dropping the constant. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Covers Min/MinNumber with a signed-zero constant, the finite opposite-signed-zero case that Runtime_98068 never exercised. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 5 pipeline(s). 10 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.
Pull request overview
This PR fixes two latent correctness issues in CoreCLR JIT’s gentree.cpp HWIntrinsic handling and adds targeted JitBlue regression coverage to prevent regressions.
Changes:
- Fix
GenTreeHWIntrinsic::GetOperForHWIntrinsicId(..., getEffectiveOp: true)to test*isScalar(not the out-param pointer) before treating certainSUBpatterns asNEG. - Fix
gtNewSimdMinMaxNode(xarch floating const-fold fast path) soneedsFixupis computed correctly for signed-zeroMin/MinNumberscenarios. - Add new JitBlue regression tests for both miscompiles.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/gentree.cpp | Correctness fixes for HWIntrinsic effective-op classification and signed-zero fixup gating in SIMD min/max constant folding. |
| src/tests/JIT/Regression/JitBlue/Runtime_130830/Runtime_130830.cs | Regression test ensuring packed integer subtract isn’t misreported as negate when only the low lane is zero. |
| src/tests/JIT/Regression/JitBlue/Runtime_130831/Runtime_130831.cs | Regression test intended to validate signed-zero behavior for Min/MinNumber with constant operands. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 1
Double/Single.Equals treat -0.0 and +0.0 as equal, so the original Assert.Equal assertions did not observe a wrong-signed-zero result. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The merged JitBlue tests need an explicit Compile entry to be built and run in CI; without it Runtime_130830 and Runtime_130831 were not exercised. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Member
Author
|
CC. @dotnet/jit-contrib, @EgorBo. Two simple bug fixes with regression tests |
tannergooding
added a commit
that referenced
this pull request
Jul 18, 2026
`Runtime_129288.cs` was added in #129348 (`JIT: don't claim rotates set ZF on xarch`) as a merged-style xunit test (`[Fact] public static int TestEntryPoint()`), but it was never referenced by any `Regression_*.csproj`. Since the SDK-style CLR test projects set `EnableDefaultItems=false`, an unreferenced `.cs` is silently never built or run -- so CI has never exercised this test. This is the same class of bug fixed in #130832. Add it to `Regression_ro_2.csproj` (an `Optimize=True` bucket, correct for this lowering/codegen correctness test), inserted in numeric order alongside its neighbors. I also audited every other `.cs` under `src/tests/JIT/Regression/` for the same issue. The only genuine orphan was `Runtime_129288`. Six other unreferenced `.cs` files are intentionally uncompiled reference sources paired with a hand-written/generated `.il` + `.ilproj` (`Runtime_70259`, `Runtime_70607`, `Runtime_73615`, `Runtime_80731`, `Runtime_40607`, `DevDiv_754566`) and were left as-is. > [!NOTE] > This PR description and the change were generated with the assistance of GitHub Copilot. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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.
Two latent correctness fixes found while auditing
gentree.cpp, each with a regression test.Fix
GetOperForHWIntrinsicIdtesting theisScalarpointer instead of*isScalar(#130830)GetOperForHWIntrinsicIdrefines aGT_SUBinto aGT_NEGwhen the constantop1is a scalar zero, but it guarded on theisScalarout-param pointer (always non-null) rather than the dereferenced value. This let a packed subtract whose constant has a zero low lane but is not all-zero (e.g.Vector128.Create(0, 1, 2, 3) - x) be reported as a negate.fgOptimizeHWIntrinsic's(-v1) + v2 => v2 - v1transform then drops the constant entirely.Fix stray block clobbering
needsFixupingtNewSimdMinMaxNode(#130831)A stray unconditional block in the min branch of the floating constant fast path overwrote
needsFixupfor all four cases, making the preceding if/else dead.needsFixupsignals that a signed-zero constant needs the AVX512 fixup somin(+0, -0)keeps the correct sign of zero; with it wrongly forcedfalse,Min/MinNumberagainst a signed-zero constant miscompiles. The min branch now mirrors the already-correct max branch.Existing coverage in
JitBlue/Runtime_98068exercisesMin/MinNumberconst-folding but always pairs an operand withNaN, so the finite opposite-signed-zero case was never tested.Both are pre-existing on
main, found by inspection. Tests added underJitBlue/Runtime_130830andJitBlue/Runtime_130831.CC. @dotnet/jit-contrib
Note
This PR description was authored with the help of GitHub Copilot.