Add IndexOfMin and IndexOfMax to hwy/contrib/algo - #3253
Merged
copybara-service[bot] merged 1 commit intoAug 10, 2026
Conversation
jan-wassenberg
approved these changes
Aug 10, 2026
jan-wassenberg
left a comment
jan-wassenberg
left a comment
Member
There was a problem hiding this comment.
Nice work! We can land as-is, I've left two minor suggestions which might help a tiny bit. More important would be to 2x unroll. Could be done in a follow-up if you like.
| IfThenElse(RebindMask(du, is_min), blocks, Set(du, LimitsMax<TU>())); | ||
| const TU min_block = ReduceMin(du, cand); | ||
| const Mask<D> winners = | ||
| And(is_min, RebindMask(d, Eq(blocks, Set(du, min_block)))); |
Member
There was a problem hiding this comment.
Minor: could be RebindMask(d, MaskedEq(RebindMask(du, is_min), blocks, Set(du, min_block))).
| const T seg_min = ReduceMin(d, acc); | ||
| const Mask<D> is_min = Eq(acc, Set(d, seg_min)); | ||
| const VFromD<decltype(du)> cand = | ||
| IfThenElse(RebindMask(du, is_min), blocks, Set(du, LimitsMax<TU>())); |
Member
There was a problem hiding this comment.
I think this could also be IfThenElseZero, then we use ReduceMax.
Contributor
Author
|
Thanks! I'll do a follow-up with both suggestions and the 2x unroll. |
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
2 participants
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.
From the op wishlist. minmax-inl.h already has MinValue/MaxValue which return the value, so these return where it is. Ties go to the lowest index, matching std::min_element.
Each lane keeps its best value plus the block it came from, then a final pass picks the smallest value, earliest block, lowest lane. Lanes hold the block number rather than the element index so it fits in the lane type — for 8-bit lanes that only reaches 255 blocks, so the loop works in segments and carries the base across. Wider lanes run as a single segment.
Roughly 10x std::min_element on 4M floats (1.46 ms vs 14.31 ms), and about 2x the cost of MinValue — the extra compare and two selects per vector, plus this isn't unrolled the way MinValue is. Happy to add the 4x unroll if you'd like it in the same PR.
Tests follow the existing minmax_value_test.cc pattern against a scalar reference, plus one that walks a single extreme across a span long enough to cross a segment boundary on 8-bit lanes.