2x unroll IndexOfMin/IndexOfMax, plus MaskedEq and ReduceMax suggestions - #3275
Open
Wint3rNight wants to merge 1 commit into
Open
2x unroll IndexOfMin/IndexOfMax, plus MaskedEq and ReduceMax suggestions#3275Wint3rNight wants to merge 1 commit into
Wint3rNight wants to merge 1 commit into
Conversation
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.
Follow-up to #3253, covering all three review comments.
MaskedEqreplacesAnd(is_min, Eq(...))in the winner resolution.IfThenElseZero+ReduceMaxreplacesIfThenElse(..., Set(du, LimitsMax<TU>()))+ReduceMin. As you noted, this needs the block counter stored inverted: blockbis now held asmax_blocks - b, so real counters occupy[1, max_blocks],0is free to mean "not a candidate", and "earliest block" becomes "largest counter", which is whatReduceMaxgives. A lane that never improved keepsmax_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 plainMin. 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: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
MinValueif you want it, though each accumulator costs another two-level tie-break merge rather than a freeMin, 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
-O2gcc 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-O3it keeps the value in a register viaminss(1.9 ms). Neither is vectorised. With both sides at-O3the real figure is ~3x forf32and ~16x fori8. 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.ccpasses 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 oni8, which has the most ties per vector).