Fix defects in __simd_find_first_of() - #2834
Open
SergeyKopienko wants to merge 30 commits into
Open
SergeyKopienko wants to merge 30 commits into
SergeyKopienko wants to merge 30 commits into
Conversation
…ype -> _DifferenceType
… in __simd_find_first_of() The __n1 < __n2 branch created the functor passed to __simd_or() once, before the loop over the first sequence, capturing __first by value. The capture kept pointing at the initial position for the whole loop, so every iteration compared the elements of the second sequence against the *first* element of the first sequence again. The brick could therefore only return two values: __first, if any element of the second sequence matches *__first, and __last otherwise - a match at any other position was never reported. Example (unseq, equality): find_first_of([1, 2], [9, 8, 7, 2]) returned index 2 (== __last) instead of index 1. The functor is now created inside the loop, directly at the __simd_or() call, so it holds the element of the current iteration; being created per iteration it also no longer needs to be mutable. The defect appeared together with the lambda in 954ce1a - before that commit the functor (__internal::__equal_value_by_pred) was constructed inside the loop.
…simd_find_first_of() [alg.find.first.of] specifies the result as the first iterator i of the first sequence for which pred(*i, *j) is true for some iterator j of the second sequence, so the element of the first sequence has to be passed as the first argument. The __n1 < __n2 branch called the predicate the other way round, pred(*j, *i). With a symmetric predicate - the common case, e.g. equality - this is invisible, which is why it went unnoticed. Any asymmetric predicate gives a wrong result, and the result also depends on the sizes of the sequences, because the __n1 >= __n2 branch of the same brick passes the arguments in the correct order. Example (unseq, pred = std::less): find_first_of([3, 9], [4, 4, 4, 4]) returned index 2 (== __last) instead of index 0, because 4 < 3 is false while 3 < 4 is true. The swapped order comes from the original PSTL code (821780f): that branch used __internal::__equal_value_by_pred, whose operator() already invoked _M_pred(__arg, _M_value), and the lambda introduced in 954ce1a kept the order.
…_of() The __n1 >= __n2 branch scans the whole first sequence once per element of the second sequence and returned the first match it found. That is the leftmost match of *some* element of the second sequence - in fact of the earliest one which matches at all - while the standard asks for the earliest position in the *first* sequence over all the elements of the second one. Example (unseq, equality): find_first_of([5, 3, 7, 9], [3, 5]) returned index 1, the match of s[0] == 3, instead of index 0, where a[0] == 5 matches s[1]. The loop now keeps the best position found so far and passes it to __simd_first() as the upper bound of the next search, so a later scan can only improve the result. Bounding the search this way is also cheaper than the original code even in the worst case: the scanned prefix shrinks monotonically instead of covering the whole first sequence __n2 times, and the loop stops as soon as position 0 is reached. When nothing is found the bound stays at __n1 and __last is returned. The defect is as old as the initial PSTL snapshot 821780f.
… additional test cases
SergeyKopienko
requested review from
MikeDvorskiy,
akukanov,
danhoeflinger and
dmitriy-sobolev
September 22, 2026 09:36
SergeyKopienko
changed the title
__simd_find_first_of()__simd_find_first_of()
Co-authored-by: Dmitriy Sobolev <Dmitriy.Sobolev@intel.com>
…nd_first_of() & introduce local variables for internal lambdas
…ew cases, consider updating the existing ones. Having a match not in the beginning is a basic scenario - it should be covered in the core tests - remove extra comments
Co-authored-by: Dmitriy Sobolev <Dmitriy.Sobolev@intel.com>
dmitriy-sobolev
previously approved these changes
Sep 22, 2026
SergeyKopienko
requested review from
dmitriy-sobolev
and
a balanced review from Copilot
September 23, 2026 15:13
Contributor
Author
|
Just for note - minimal fix required the state from commit |
…ngth, without the block begin index" This reverts commit fde5389.
This branch has not been deployed
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.
Found in #2832; the tests in
maindid not catch it.Fixed defects in
__simd_find_first_of()__n1 < __n2): the__simd_or()functor captured__firstonce, before the loop.find_first_of([1, 2], [9, 8, 7, 2])returnedlast.__n1 < __n2): [alg.find.first.of] requirespred(*i, *j). Withstd::less,find_first_of([3, 9], [4, 4, 4, 4])returnedlast.__n1 >= __n2):find_first_of([5, 3, 7, 9], [3, 5])returned index 1 instead of 0.Design (review follow-up)
The first sequence is scanned in blocks of 256 bytes doubling up to 16 KB, and the scan stops at the first block with a match. Each block is searched with the two original strategies, choosing by whether the block or the second sequence is longer. As a result:
n2;counting_iterator<std::int8_t>, do not overflow.Tests
find_first_of.pass.cppcovers each defect and now fails onmain:CallIdparameter that keeps the SYCL kernel names unique.implementation_details/simd_find_first_of.pass.cppchecks narrow difference types on the longest representable first sequence.