Skip to content

Fix SIMD MinMax constant special cases - #133173

Open
tannergooding wants to merge 3 commits into
dotnet:mainfrom
tannergooding:tannergooding-fix-jit-constant-special-cases
Open

Fix SIMD MinMax constant special cases#133173
tannergooding wants to merge 3 commits into
dotnet:mainfrom
tannergooding:tannergooding-fix-jit-constant-special-cases

Conversation

@tannergooding

Copy link
Copy Markdown
Member

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.

Fixes #133022

Note

This pull request was created with GitHub Copilot.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 3, 2026 15:07
@github-actions github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 3, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

It changes low-level JIT SIMD constant handling and codegen-path selection, so a final human review is needed to validate cross-architecture/codegen implications despite the added regression test.

Review tier: Lite
Findings: None

What changed in this PR

This PR updates CoreCLR JIT SIMD Min/Max handling so constant-lane “special values” (NaN and signed zero) are detected per-lane (any-lane) rather than requiring whole-vector uniformity, preventing incorrect selection of an uncompensated native min/max fast-path for mixed constants. It also adds a regression test covering NaN propagation and signed-zero behavior for Vector128/Vector256 Min/Max and MinNumber/MaxNumber scenarios.

Changes:

  • Extend SIMD constant-evaluation helpers to support partial-vector sizes and add reusable mask predicates (IsNaN, IsNegative, Is(±)Zero, any/all mask checks).
  • Add GenTreeVecCon predicates for “contains any NaN / ±0 lane” and use them in the xarch Min/Max fast-path eligibility logic.
  • Add a new JIT regression test and wire it into the merged regression csproj.
File Description
src/​coreclr/​jit/​simd.h Adds SIMD mask helper predicates and any/all mask evaluation used to reason about special lanes in constants.
src/​coreclr/​jit/​gentree.h Declares new GenTreeVecCon “contains special lane” helpers used by Min/Max handling.
src/​coreclr/​jit/​gentree.cpp Implements the new predicates and updates Min/Max constant-path fixup gating to use any-lane NaN/±0 detection.
src/​tests/​JIT/​Regression/​JitBlue/​Runtime_133022/​Runtime_133022.cs Adds coverage for NaN propagation and signed-zero semantics across Min/Max and MinNumber/MaxNumber, including constant-vs-opaque operand cases.
src/​tests/​JIT/​Regression/​Regression_ro_2.csproj Includes the new regression test in the merged JIT regression project.

@AndyAyersMS

AndyAyersMS commented Sep 3, 2026

Copy link
Copy Markdown
Member

MinNumber/MaxNumber const-fold: this change fixes the non-AVX512 path but regresses AVX512, and the new test fails on AVX512 hardware

I ran this PR on an AVX512 box (Xeon Platinum 8370C, Windows x64, .NET 11 Checked). Summary: with DOTNET_EnableAVX512=0 the change is a complete fix; with AVX512 enabled it does not fix the original issue and introduces a new wrong-value fold.

Method. Full build.cmd clr+libs -rc Checked -lc Release at PR head f3bc6de, then a second Core_Root produced by reverting gentree.cpp / gentree.h / simd.h to the PR base 719009a and rebuilding only clr.jit. I hash-compared both Core_Root trees: clrjit.dll is the only differing file, so the JIT change is the sole variable.

1. The PR's own regression test fails on AVX512

JIT AVX512 Runtime_133022
base 719009a on 2 failures
base 719009a off 2 failures
f3bc6de on 2 failures
f3bc6de off 0 failures

2. New wrong value under AVX512

Vector128<double>.MinNumber where the left operand is a constant containing a NaN and the right operand is opaque:

a = (0x7FF8000000000001 /* NaN  */, 0xFFF0000000000000 /* -Inf */)   constant
b = (0x7FF8000000000000 /* NaN  */, 0xFFF8000000000000 /* -NaN */)   opaque
operand delivery base 719009a f3bc6de
both constant [7FF8000000000001, FFF0000000000000] [7FF8000000000001, FFF0000000000000]
left const, right opaque [7FF8000000000001, FFF0000000000000] [7FF8000000000000, FFF8000000000000]
left opaque, right const [7FF8000000000001, FFF0000000000000] [7FF8000000000001, FFF0000000000000]
both opaque (reference) [7FF8000000000001, FFF0000000000000] [7FF8000000000001, FFF0000000000000]

The folded result becomes the other operand wholesale. Lane 1 is the real problem and is not a NaN-payload nicety: MinNumber(-Inf, -NaN) must be -Inf, and it now yields -NaN.

Passes on 719009a at both gates and on f3bc6de with DOTNET_EnableAVX512=0; fails only on f3bc6de with AVX512 enabled. Needs a warmup loop so the method reaches tier 1.

standalone repro (exit 100 = pass, 101 = fail) — verified: PASS on base at both gates and on the PR with AVX512 off, FAIL only on the PR with AVX512 on
using System;
using System.Runtime.CompilerServices;
using System.Runtime.Intrinsics;

public static class Program
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    private static double O(double v) => v;              // opaque delivery

    private static ulong Canon(double d)                 // canonicalize NaN payloads away
        => double.IsNaN(d) ? 0x7FF8000000000000UL : BitConverter.DoubleToUInt64Bits(d);

    private static ulong H(Vector128<double> v)
    {
        ulong lo = Canon(v.GetElement(0)), hi = Canon(v.GetElement(1));
        return lo ^ ((hi << 32) | (hi >> 32));
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static bool Agree()
    {
        // NOTE: the constants must be written inline; routing them through a local
        // or a helper defeats the fold and the test silently becomes all-opaque.
        var bothConst = Vector128.MinNumber(
            Vector128.Create(BitConverter.UInt64BitsToDouble(0x7FF8000000000001UL), BitConverter.UInt64BitsToDouble(0xFFF0000000000000UL)),
            Vector128.Create(BitConverter.UInt64BitsToDouble(0x7FF8000000000000UL), BitConverter.UInt64BitsToDouble(0xFFF8000000000000UL)));

        var rightOpaque = Vector128.MinNumber(
            Vector128.Create(BitConverter.UInt64BitsToDouble(0x7FF8000000000001UL), BitConverter.UInt64BitsToDouble(0xFFF0000000000000UL)),
            Vector128.Create(O(BitConverter.UInt64BitsToDouble(0x7FF8000000000000UL)), O(BitConverter.UInt64BitsToDouble(0xFFF8000000000000UL))));

        var leftOpaque = Vector128.MinNumber(
            Vector128.Create(O(BitConverter.UInt64BitsToDouble(0x7FF8000000000001UL)), O(BitConverter.UInt64BitsToDouble(0xFFF0000000000000UL))),
            Vector128.Create(BitConverter.UInt64BitsToDouble(0x7FF8000000000000UL), BitConverter.UInt64BitsToDouble(0xFFF8000000000000UL)));

        var reference = Vector128.MinNumber(
            Vector128.Create(O(BitConverter.UInt64BitsToDouble(0x7FF8000000000001UL)), O(BitConverter.UInt64BitsToDouble(0xFFF0000000000000UL))),
            Vector128.Create(O(BitConverter.UInt64BitsToDouble(0x7FF8000000000000UL)), O(BitConverter.UInt64BitsToDouble(0xFFF8000000000000UL))));

        ulong h = H(reference);
        return H(bothConst) == h && H(rightOpaque) == h && H(leftOpaque) == h;
    }

    public static int Main()
    {
        bool ok = true;
        for (int i = 0; i < 200_000; i++) ok = Agree();   // warm to tier 1
        Console.WriteLine(ok ? "PASS" : "FAIL: operand-delivery shapes disagree");
        return ok ? 100 : 101;
    }
}

3. At scale

A differential fuzzer that emits the same computation in all four operand-delivery shapes and compares them, 300 seeds, identical seeds on both JITs:

JIT AVX512 on AVX512 off
base 719009a 274 / 300 disagree 222 / 300 disagree
f3bc6de 290 / 300 disagree 0 / 300

Per seed, 21 go clean -> broken and 5 broken -> clean. So the non-AVX512 result is unambiguous (222 -> 0) and AVX512 is a net regression rather than merely unimproved.

Lead, not a conclusion: the failing shape is exactly "constant operand contains a NaN, other operand opaque", which is what the new GenTreeVecCon::ContainsNaN / needsFixup predicate newly enables. I have not read the resulting codegen, so I can't say whether the bug is in the predicate or in the AVX512 lowering it now reaches.

Scope confirmed only for Vector128<double>.MinNumber under fullopts; I have not validated 256/512 or MaxNumber.

Happy to run any experiment on this hardware if it helps.

Note

This comment was generated by GitHub Copilot.

@AndyAyersMS

Copy link
Copy Markdown
Member

My fuzzing indicates AVX-512 sensitivity here... can we also add some tests for it?

…the result

The single constant AVX-512 Fixup escape hatch was broken three ways: the table was broadcast, so a non-uniform constant had the zero sign forced in every element rather than only the ones that needed it; the isNumber operand swap overwrote the Fixup node's own operands; and the ZERO fixup token does not distinguish +0 from -0, so it cannot produce the sign-dependent answer that MinNumber/MaxNumber require.

Build the table per element so mixed constants are handled, restrict the fixup to the Min/Max cases it can actually express, and let everything else fall through to the general handling. A partially NaN constant cannot use the operand ordering either, so it falls through as well.

Also prefer the general IR over the AVX-512 Range/Fixup sequence when both inputs are constant, since Range/Fixup does not constant fold.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@tannergooding

Copy link
Copy Markdown
Member Author

Yeah, I added some comprehensive tests covering the various edges.

I hadn't looked closely enough at the vfixupimm path and so missed that it was also only setup to handle the scalar case. Have it properly handling things now and has been tested on AVX and AVX512 for scalar and vector across the various edges.

Add general Min/Max semantic coverage for both NaN signs and a focused mixed-lane regression for the single-constant optimization.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 4, 2026 02:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The updated Min/Max special-casing logic is still unreachable for the “both operands are constant vectors” scenario central to #133022, so the regression may remain unfixed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1 High severity · 1 Medium severity

New issues introduced by this change (2)
Severity Finding
High severity src/​coreclr/​jit/​gentree.cpp — This block’s logic is still only reached when exactly one operand is constant (`else if ((cnsNode…
Medium severity src/​coreclr/​jit/​gentree.cpphasPartialNaN is documented as tracking partially NaN constants, but the current predicate is…

Comment thread src/coreclr/jit/gentree.cpp
Comment thread src/coreclr/jit/gentree.cpp
@tannergooding

Copy link
Copy Markdown
Member Author

The previous feedback was both false positives.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JIT: Vector128.Min/Max emit an uncompensated vminpd and lose NaN propagation when both operands are constant (regression from .NET 9)

3 participants