Remove unsafe code from System.Linq.MaxMin

[EgorBo]

Note

This PR is AI-generated.

This PR does several things:

1. Replace unsafe code with safe in MinMaxInteger
2. Replace scalar loops over Vector512 and Vector256 with a reduction to Vector128 so we only run scalar loop over the smallest vector - this improves perf.
3. Adds a bunch of [AI] to satisfy inliner

(3) is needed because we run out of inliner budget in the safe version: https://gist.github.com/EgorBo/278e4bc6795cc829b9e129d7c5932f14

Benchmark

public class Bench
{
    private int[] _ints = default!;
    private long[] _longs = default!;
    private byte[] _bytes = default!;

    [Params(2, 4, 16, 32, 200, 1000)]
    public int Length { get; set; }

    [GlobalSetup]
    public void Setup()
    {
        var rng = new Random(42);
        _ints = new int[Length];
        _longs = new long[Length];
        _bytes = new byte[Length];
        for (int i = 0; i < Length; i++)
        {
            _ints[i] = rng.Next();
            _longs[i] = ((long)rng.Next() << 32) | (uint)rng.Next();
            _bytes[i] = (byte)rng.Next(256);
        }
    }

    [Benchmark] public int IntMax() => _ints.Max();
    [Benchmark] public int IntMin() => _ints.Min();
    [Benchmark] public long LongMax() => _longs.Max();
    [Benchmark] public byte ByteMax() => _bytes.Max();
}

Summary — speedup of PR vs main (>1 = PR faster, <1 = PR slower). Full results: EgorBot/Benchmarks#187.

x64 (AMD EPYC, AVX-512) — significant wins from inliner-budget fixes + more efficient Vector256/512 → scalar reduction:

Method	2	4	16	32	200	1000
IntMax	1.03	1.07	9.41	2.79	5.42	1.89
IntMin	1.02	1.07	3.10	2.61	3.03	1.22
LongMax	1.15	1.92	9.19	2.41	1.06	1.06
ByteMax	2.42	3.37	1.26	3.03	6.52	7.76

arm64 (Apple M2, Vector128) — mostly flat; minor regressions at small/medium lengths

Method	2	4	16	32	200	1000
IntMax	0.92	0.95	0.83	0.99	1.08	1.01
IntMin	0.96	0.99	0.83	0.98	1.05	1.02
LongMax	1.03	0.91	0.99	0.99	1.00	1.00
ByteMax	0.99	1.00	0.98	1.00	0.92	1.03

[EgorBo]

@MihuBot

[EgorBo]

@EgorBot -arm -amd

using System;
using System.Linq;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(typeof(Bench).Assembly).Run(args);

public class Bench
{
    private int[] _ints = default!;
    private long[] _longs = default!;
    private byte[] _bytes = default!;

    [Params(2, 4, 16, 32, 200, 1000)]
    public int Length { get; set; }

    [GlobalSetup]
    public void Setup()
    {
        var rng = new Random(42);
        _ints = new int[Length];
        _longs = new long[Length];
        _bytes = new byte[Length];
        for (int i = 0; i < Length; i++)
        {
            _ints[i] = rng.Next();
            _longs[i] = ((long)rng.Next() << 32) | (uint)rng.Next();
            _bytes[i] = (byte)rng.Next(256);
        }
    }

    [Benchmark] public int IntMax() => _ints.Max();
    [Benchmark] public int IntMin() => _ints.Min();
    [Benchmark] public long LongMax() => _longs.Max();
    [Benchmark] public byte ByteMax() => _bytes.Max();
}

[EgorBo]

PTAL @MihaZupan @tannergooding

Had to mark the method NoInlining due to inliner issues I want to address separately. Currently it adds 0.5ns call overhead for small inputs (but only for trivial benchmark wrappers where the entire thing used to inline, in the real world it's very unlikely to happen) Otherwise, no extra bound checks in this impl in codegen.

benchmark: #127845 (comment) and results: EgorBot/Benchmarks#186
Significant wins on x64 due to inliner budget issues + more efficient "vector to scalar" path for Vector256 and Vector512.

Follow ups:

• Improve inliner and remove NoInlining
• Improve Vector128 to scalar for Max/Min via shuffle (wasn't the goal of this PR)

[MihaZupan]

GitHub reviews are broken atm, so here's the comment I wanted to post:

Does this do anything since it's on the interface?

For the MethodImpl on private interface IMinMaxCalc<T>

[EgorBo]

GitHub reviews are broken atm, so here's the comment I wanted to post:

Does this do anything since it's on the interface?

For the MethodImpl on private interface IMinMaxCalc<T>

In this method we currently still run out of time budget, [AI] gave inliner a hint that we should inline this 14b method even we when already ran out of time budget. Otherwise I see non-inline calls in the codegen.

Also something I want to clean up in a separate inliner change. Although, in this case we just need to increase the budget based on some heuristics like Vector.IsSupported branches.

[MihaZupan]

I meant specifically about adding the attribute on the interface's abstract definition itself.
Does the JIT look at that at all? I had assumed it'd only care about the implementation types.

[tannergooding]

Are these not within the "always inline" threshold already?

I would've though this is like 5-6 bytes of IL and so something that we should trivially believe is in budget, particularly for primitive T

Maybe it being an op_GreaterThan constrained call is what's throwing things off?

[EgorBo]

yeah this one was 14 bytes of IL, and we reduce "always inline" to 12 bytes once we ran out of time budget.

I removed this AggressiveInlining as it's no longer relevant with #127845 (comment)

[tannergooding]

Interesting, I wonder if we should special case some of the IL sizes here like this one where it's namely big because of the constrained. <token> prefix.

We could of course special case all the generic math calls for the primitives as well, but it'd be nice if we didn't need to do that and have the inliner "do the right thing".

[tannergooding]

Maybe in T1 we could do something like "inline everything <= 12 bytes" and then look at IL to make a heuristic for everything <= otherBound", such as if it is a single call or other special cases that are "likely profitable" but slightly larger.

[EgorBo]

@MihaZupan @tannergooding so I decided to take another approach: if we already happen to inline a method with Vector<ANY>.IsSupported\IsHardwareSupported (or had it in the root compilation) we increase our time budget by 5x. The rationale is that all functions with different SIMD paths (e.g. a big block for Vector128, then for 256, then 512, etc) are IL heavy and contain tons of small calls and we'd better inline them all (but still not with infinite budget, just in case). This should allow me to remove all the ugly workarounds. Although, this is also a bit ad-hoc heuristic.

[EgorBo]

@EgorBot -arm -amd

using System;
using System.Linq;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(typeof(Bench).Assembly).Run(args);

public class Bench
{
    private int[] _ints = default!;
    private long[] _longs = default!;
    private byte[] _bytes = default!;

    [Params(2, 4, 16, 32, 200, 1000)]
    public int Length { get; set; }

    [GlobalSetup]
    public void Setup()
    {
        var rng = new Random(42);
        _ints = new int[Length];
        _longs = new long[Length];
        _bytes = new byte[Length];
        for (int i = 0; i < Length; i++)
        {
            _ints[i] = rng.Next();
            _longs[i] = ((long)rng.Next() << 32) | (uint)rng.Next();
            _bytes[i] = (byte)rng.Next(256);
        }
    }

    [Benchmark] public int IntMax() => _ints.Max();
    [Benchmark] public int IntMin() => _ints.Min();
    [Benchmark] public long LongMax() => _longs.Max();
    [Benchmark] public byte ByteMax() => _bytes.Max();
}

[EgorBo]

PTAL @AndyAyersMS @dotnet/jit-contrib inliner changes please. I need this in other places as well, my work to replace unsafe code with safe too often runs into "out of time budget". Functions with SIMD are typically very rare outside of BCL so I don't expect any impact on users codebases.

[EgorBo]

benchmarks seem happy without all the MethodImpl hacks EgorBot/Benchmarks#187

[tannergooding]

Do we want to make this observation for the false case, since presumably everything under that path is "dead code"?

[tannergooding]

Similar question here, do we want to make this observation in the case the only path is for an unsupported type? Will this negatively impact for example when T is System.__Canon and so all the intrinsics paths are dead/throwing?

[tannergooding]

We still don't have a way to adjust how much budget was estimated to be used vs actually got used in the case most imported code is dead, right?

[AndyAyersMS]

We do make this adjustement:

runtime/src/coreclr/jit/flowgraph.cpp

Lines 570 to 583 in 4ffb643

	// Leave a note if we only did a partial import.
	if (importedILSize != info.compILCodeSize)
	{
	JITDUMP("\n** Note: %s IL was partially imported -- imported %u of %u bytes of method IL\n",
	compIsForInlining() ? "inlinee" : "root method", importedILSize, info.compILCodeSize);
	}
	// Record this for diagnostics and for the inliner's budget computations
	info.compILImportSize = importedILSize;
	if (compIsForInlining())
	{
	compInlineResult->SetImportedILSize(info.compILImportSize);
	}

This ultimately feeds into the budget update.