Skip to content

2x unroll IndexOfMin/IndexOfMax, plus MaskedEq and ReduceMax suggestions - #3275

Open
Wint3rNight wants to merge 1 commit into
google:masterfrom
Wint3rNight:index-of-min-max-unroll
Open

2x unroll IndexOfMin/IndexOfMax, plus MaskedEq and ReduceMax suggestions#3275
Wint3rNight wants to merge 1 commit into
google:masterfrom
Wint3rNight:index-of-min-max-unroll

Conversation

@Wint3rNight

@Wint3rNight Wint3rNight commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #3253, covering all three review comments.

MaskedEq replaces And(is_min, Eq(...)) in the winner resolution.

IfThenElseZero + ReduceMax replaces IfThenElse(..., Set(du, LimitsMax<TU>())) + ReduceMin. As you noted, this needs the block counter stored inverted: block b is now held as max_blocks - b, so real counters occupy [1, max_blocks], 0 is free to mean "not a candidate", and "earliest block" becomes "largest counter", which is what ReduceMax gives. A lane that never improved keeps max_blocks, i.e. block 0, correct for an all-ties span. There was no test for that case, so I added one.

2x unroll. Two accumulators, even blocks in one and odd in the other. The merge has to carry the block along with the value and keep resolving ties to the earlier block, so it is Lt(acc1, acc0) || (Eq(acc0, acc1) && Gt(blocks1, blocks0)) rather than a plain Min. The bulk loop also stops calling `LoadNOr', it has a full-vector fast path, but the length computation and its branch still run every iteration; only the tail needs it now.

Benchmarks, AVX2 on a Zen 3 (Ryzen 5 5600H), both implementations in one binary alternating order, best of 9, everything at -O3. The middle column is the load change alone with a single accumulator, to separate it from the unroll:

merged 1 acc, unmasked bulk 2 acc, unmasked bulk
f32, 64 MB 2.679 ms 2.651 ms (1.01x) 2.621 ms (1.02x)
f32, 16 MB 0.591 ms 0.537 ms (1.10x) 0.539 ms (1.10x)
f32, 4 MB 0.078 ms 0.088 ms (0.89x) 0.075 ms (1.04x)
f32, 256 KB 0.005 ms 0.006 ms (0.86x) 0.004 ms (1.10x)
i8, 16 MB 0.618 ms 0.514 ms (1.20x) 0.494 ms (1.25x)
i8, 1 MB 0.030 ms 0.020 ms (1.46x) 0.018 ms (1.63x)
i32, 16 MB 0.692 ms 0.626 ms (1.10x) 0.601 ms (1.15x)

Most of the gain is the load change; the second accumulator adds 1.02–1.12x on top. It earns its place mainly at the small sizes, where the load split alone regresses (0.86x at 256 KB) because the extra loop isn't amortised. At genuinely DRAM-bound sizes none of it moves (1.02x at 64 MB). Happy to take this to 4x like MinValue if you want it, though each accumulator costs another two-level tie-break merge rather than a free Min, so I'd expect it to flatten out.

One correction to #3253 while I'm here: the "roughly 10x std::min_element" in that PR body was measuring the compiler, not the kernel. The scalar baseline was built at a lower optimisation level — at -O2 gcc keeps the running minimum in memory and reloads it through the result pointer each iteration, so it pays load-use latency (13.5 ms), while at -O3 it keeps the value in a register via minss (1.9 ms). Neither is vectorised. With both sides at -O3 the real figure is ~3x for f32 and ~16x for i8. Same paragraph also called 16 MB "past L3" on a machine whose L3 is exactly 16 MiB. Apologies for the noise in the record.

Tests: existing minmax_value_test.cc passes 25/25 on AVX2, SSE4, SSSE3, SSE2 and EMU128, plus the new all-identity case. I also injected three deliberate bugs into the new merge, dropping the tie-break clause, inverting it, and storing the wrong block number in the odd stream, and confirmed each is caught (all three surface on i8, which has the most ties per vector).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant