Fix incorrect constant folding of CompareScalar True/False comparisons#130222
Open
tannergooding wants to merge 2 commits into
Open
Fix incorrect constant folding of CompareScalar True/False comparisons#130222tannergooding wants to merge 2 commits into
tannergooding wants to merge 2 commits into
Conversation
The True/False float comparison modes were constant folded to a full Zero/AllBitsSet result, which is wrong for CompareScalar since it must copy the upper elements unchanged from op1. Also, gtNewZeroConNode and gtNewAllBitsSetConNode did not support TYP_MASK, crashing the AVX512 mask compare fold. - Add TYP_MASK support to gtNewZeroConNode. - Only fold CompareScalar when op1 is a constant, setting just the lowest element and preserving the upper elements. - Build a canonical true mask (AllBitsSet(elementCount)) for mask results instead of relying on gtNewAllBitsSetConNode. - Add regression test Runtime_125160. Fixes dotnet#125160 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR corrects JIT constant-folding for AVX compare intrinsics in the “always true/always false” comparison modes, ensuring Avx.CompareScalar preserves the upper elements from op1 and avoiding crashes when folding AVX512 mask-typed compares.
Changes:
- Extend
gtNewZeroConNodeto supportTYP_MASK(viaGenTreeMskCon) underFEATURE_MASKED_HW_INTRINSICS. - Update
gtFoldExprHWIntrinsicfolding forNI_AVX_Compare/NI_AVX_CompareScalar/NI_AVX512_CompareMask, with special handling forCompareScalarto only fold whenop1is a constant vector and to preserve upper elements. - Add and wire up a new JIT regression test
Runtime_125160.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/gentree.cpp | Fixes compare folding behavior for scalar compares and makes zero-constant creation mask-safe. |
| src/tests/JIT/Regression/JitBlue/Runtime_125160/Runtime_125160.cs | Adds regression coverage for CompareScalar true/false modes and AVX512 compare folding scenarios. |
| src/tests/JIT/Regression/Regression_ro_2.csproj | Includes the new Runtime_125160 test in the regression suite. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 3
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
161099e to
56316fc
Compare
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.
Why
Avx.CompareScalarwith a constant "always true"/"always false" float comparison mode (OrderedFalse*,UnorderedTrue*) was being constant folded to a fullZero/AllBitsSetvector. That is wrong for the scalar form:CompareScalaronly writes the lowest element and must copy the remaining upper elements unchanged fromop1. The fold discarded those upper elements, producing incorrect results (e.g.CompareScalar(<1,2,3,4>, ..., OrderedFalse)yielded<0,0,0,0>instead of<0,2,3,4>).Separately,
gtNewZeroConNodeandgtNewAllBitsSetConNodedid not supportTYP_MASK(they hitunreached()), so folding the AVX512 mask compare (NI_AVX512_CompareMask, whose result type isTYP_MASK) for these modes crashed.Fixes #125160
Approach
gtNewZeroConNodenow handlesTYP_MASKby returning a zeroGenTreeMskCon(guarded byFEATURE_MASKED_HW_INTRINSICS).gtFoldExprHWIntrinsicTrue/False handling forNI_AVX_Compare/NI_AVX_CompareScalar/NI_AVX512_CompareMask:CompareScalar, bail out of folding entirely unlessop1is itself a constant. When it is, set only the lowest element (0for false modes, all-ones for true modes) and leave the upper elements ofop1intact.gtNewZeroConNode(retType).simdmask_t::AllBitsSet(elementCount)(matchingVNAllBitsForType) rather than relying ongtNewAllBitsSetConNode, which cannot know the element count. Vector (non-mask) results still usegtNewAllBitsSetConNode.Notes
op1node as the result and wraps it withgtWrapWithSideEffects(tree, ...); becauseop1is a side-effect-free constant, this preserves any side effects from the other operands without duplicatingop1.gtSimdVal.u32[0]/u64[0]) are used for the true-mode all-ones pattern sinceSetElementIntegralasserts on floating base types.Testing
Added
Runtime_125160regression test covering all four True/False modes for float and doubleAvx.CompareScalar(asserting both the low-element result and upper-element preservation) plusAvx512F.CompareforVector512<float>/<double>. Verified the test fails without the fix (Expected2, Actual0) and passes with it. Baselineclr+libs(checked) build is clean.Note
This pull request was authored by GitHub Copilot.