This test (and the other removed test) didn't check the actual boundary condition. For example, if NMax is changed to 8192 in the Adler32 implementation, the tests still pass.

The updated test correctly breaks if NMax is set as small as 5553.

Why separate it out like this?

The JIT tends to special case 1 level of inlining differently from 2+ levels of inlining and so simple forwarders like this can hurt things more than help.

The SIMD implementation is all in file-scoped types, so it has to be called from something in this file. I could make those types nested private, but since there are so many, I was trying to keep them entirely local. If you prefer the nested approach, I can easily change it, though I don't foresee any issues with inlining limits here given the core method is intentionally marked NoInlining.

What hardware were you testing AVX512 on? I wouldn't expect it to be slower than Vector256 at all and at worst the same speed here.

I'm on an AMD Zen5 (see full benchmark details in the PR description).

The AVX-512 implementation adds some extra high-latency calculations to the inner loop, so it's expected to be slower. It can't be made to match the perf of AVX2 using the same logic widened, because vpmaddubsw will overflow with the larger multipliers required for the wider inputs, and any adjustment made to prevent overflow breaks the pipelining. The whole thing is very latency-sensitive.

A fast Vector512 implementation could be done with the AVX10.2 unsigned dot product instructions (vpdpbuud), but I didn't bother with that since the hardware still doesn't exist in the wild. I could add it preemptively if you like.

You should be able to treat it as 2x256 instead of as 1x512 to avoid the wider multiplier issue and still get the perf gains.

Yes, that's what this PR does to improve 2x over main. Main uses 1x256 or 1x512. 2x256 is faster than either.

I would expect that actual 2x256 (i.e. effectively unrolling) should be slightly slower than using actual 512 and treating it as 2x256, namely due to the denser code and not needing to manually pipeline the instructions. I would not expect the V512 path to be slower.

This already gets instruction-level parallelism. At best, you could match the perf with V512, but that's a lot of extra complexity for nothing.

What is the perf difference between these two paths? Is it worth the additional complexity here?

The DP implementation is roughly twice as fast as base AdvSimd. Without it, this PR is only about 5% faster than Main on Arm64.

What is the perf difference of the above code paths with the xplat path (all platforms)?

Xplat is around 1/3 the speed of native on both x64 (if restricted to Vector128) and Arm64 (if restricted to AdvSimd base).

why not Vector128.Create<uint>(ushort.MaxValue)?

I actually wrote the code as I wanted it to be interpreted by JIT, i.e. pcmpeqd+psrld, but it gets constant folded and treated as a memory load anyway.

We're looking at ways to reduce or otherwise remove unsafe code like this. While we can't really remove LoadUnsafe, we have found it significantly less error-prone to never update sourceRef and to track the relevant offset indices instead, as it reduces risk of accidental GC holes.

This is the exact same reference math done in the current implementation. What's changed between last week when that was approved and now?

A comment was given then too. Last week was namely just taking the existing code "as is" and extending it for the parameterization.

This is touching the code with a slightly more significant and non-critical rewrite (even with quite a lot of it being the same and just moved down for sharing). Since we're actively doing work to reduce unsafe usage where feasible, then ideally we fix this up rather than continuing to persist the problematic code.

I think you've mixed up the Adler32 and parameterized CRC32/64 PRs. Vectorized Adler32 was 100% new in #124409.

It's certainly possible to move to a buffer offset, but I think any code using LoadUnsafe requires scrutiny, and it's trivially provable that this code does not have the potential to create a GC hole.

Shouldn't this second one be HorizontalAddSaturate. The multiply by 1 is unnecessarily adding 2 cycles.

It's a widen + add pairwise. There's actually a single dot product instruction that does it all in AvxVnni (vpdpbusd), but that creates a dependency on the uint accumulator that ends up making it just slightly slower than this form on the two Intel machines and one AMD machine I tried. Though, as mentioned above, I believe the unsigned form of dot product could be used to make a faster Vector512 implementation.

It's a widen + add pairwise

Right, but the whole setup here is effectively just doing Sum(left * right) (reducing down to multiple 32-bit results, instead of one 8-bit result), which I'm pretty sure can be simplified to less than 5 cycles on anything Skylake or newer

Right, and that's a dot product. The only widening dot product instructions I'm familiar with for x86 are VNNI. What exact instruction sequence are you thinking of?

Why do we need to be doing a full reduction every loop iteration here for wps?

It seems like a simply widen + add and then only reduce outside the loop should be plenty sufficient here and likely provide a bigger perf win.

These are different accumulators that move at different rates. The only easy thing to factor out is the multiplication of the previous sum by the number of bytes that it would be added to each iteration, and that's already done.

Similar here. This is effectively just doing ws2 + Sum(bytes1 * weights1) and ws3 + Sum(bytes2 * weights2)

It isn't clear why the sum at this point is actually needed every inner loop iteration and why it couldn't be hoisted "out" so that it's done only in the outer loop

It could be hoisted out, but then you still have to widen each element before accumulating, which is still expensive. See the first attempt at Arm64 acceleration in Stephen's PR for an idea what that looks like. It's not faster.

Widening is significantly cheaper and more pipelineable (and at least on AVX512 has single instruction, single cycle versions that goto wider registers).

I would expect decent savings if we were only widening and not doing the reductions per inner loop iteration, particularly that would simplify the algorithm and allow better code sharing across all these platforms.

I gave that a quick try again (tried it before a long time ago but didn't keep the result as it wasn't worthwhile). It's still slower.

If you think you can do better than this implementation, be my guest. This was the best perf I could get, on every platform.

We're notably not strictly looking for the "best perf" on every platform.

Rather, we're looking for good enough perf given the complexity, expected payload sizes, and real world impact (not every function is going to be a bottleneck or matter if its taking 200ns vs 400ns).

So part of what's being considered here is whether the extra code complexity, generics, impact to NAOT image size, or other scenarios, etc are meaningful enough compared to just having the simpler and very slightly slower code.

-- With this being a case where I expect we can remove most of the per ISA customization and still get "close enough" or even matching on most hardware, particularly when not simply looking at "latest" Intel or AMD and rather at the broad range of typical hardware targets which tend to be a bit older (Haswell, Skylake, Ice Lake, Zen 2, etc).

extra code complexity

Although this is more lines of code, I'd argue it's less complex, simply because it's more consistent. The current implementation uses different logic for different vector widths, inconsistent variable names, etc. The new implementation uses the same skeleton for all, with only very small kernels abstracted away per-platform and with names that are easy to follow.

impact to NAOT image size

It should be noted, this generic approach is an improvement for NAOT code size, because e.g. on x64, instead of dynamically dispatching between up to 3 different implementations depending on ISA support and input size, this will choose exactly 1, which will always be used for any input >= 16 bytes.

In the case of Arm64, it will compile up to 2 potential versions of the core method, but it moves the ISA check out to the dispatcher, avoiding dynamic checks in the inner loop. And if 2x performance isn't good enough to justify a second copy, why are we bothering to implement Vector256 (not to mention the Vector512 implementation I got rid of and which you've argued to bring back)?