Fix missing bytesize check for safe find vectorization with LLVM#6298
Fix missing bytesize check for safe find vectorization with LLVM#6298mgovers wants to merge 2 commits into
find vectorization with LLVM#6298Conversation
… LLVM Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds coverage and refines internal gating for vectorized algorithms by centralizing “allowed element sizes” and tightening _Vector_alg_in_find_is_safe eligibility.
Changes:
- Introduces
_Is_vector_element_sizeand reuses it across reverse/reverse_copy implementations. - Tightens
_Vector_alg_in_find_is_safe_elemby requiring element sizes of 1/2/4/8. - Adds a new compile-only test for
_Vector_alg_in_find_is_safeand extends existing_Equal_memcmp_is_safe/_Vector_alg_in_search_is_safecoverage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/std/tests/GH_006294_vectorized_find_is_safe/test.compile.pass.cpp | New compile-time assertions for _Vector_alg_in_find_is_safe across scalar/pointer/custom types. |
| tests/std/tests/GH_006294_vectorized_find_is_safe/env.lst | Wires the new test directory into the usual test matrix. |
| tests/std/tests/GH_000431_equal_memcmp_is_safe/test.compile.pass.cpp | Adds a larger default-comparison type and validates memcmp/search safety expectations. |
| tests/std/test.lst | Registers the new GH_006294 test in the overall test list. |
| stl/inc/xutility | Centralizes size gating via _Is_vector_element_size; tightens find safety; reuses gating in reverse. |
| stl/inc/algorithm | Reuses _Is_vector_element_size gating in ranges::reverse, reverse_copy, and ranges::reverse_copy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #ifdef __clang__ | ||
| constexpr bool vectorized_trivial_comparison = true; | ||
| #else | ||
| constexpr bool vectorized_trivial_comparison = false; | ||
| #endif |
There was a problem hiding this comment.
I don't think there's such a trait. Instead, since the actual implementation uses #ifdef __clang__, I believe that this approach is the correct one. It also aligns with #5767
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
|
/azp run STL-CI |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The failing tests are x86_86-only: However, I do not have an x86 machine. How can I investigate what went wrong? Regarding the CLA, I did not forget about it, I'm on it. |
I am now able to run the x86 tests but don't seem to be able to reproduce the error... from an x86 native tools command prompt, running for all failing tests in the above: <...>\STL\out\x86> python tests\utils\stl-lit\stl-lit.py -Dnotags=ASAN ..\..\tests\<test_dir>\yields PASS: std :: tests/Dev11_0732166_unordered_strong_guarantee:41 (32 of 46)
PASS: std :: tests/P0009R18_mdspan_layout_right:21 (27 of 30)
PASS: std :: tests/P1206R7_unordered_multiset_from_range:27 (28 of 30)
PASS: std :: tests/P2286R8_text_formatting_range_formatter:21 (26 of 30)
PASS: std :: tests/P2321R2_views_zip_transform:027 (52 of 116)
PASS: std :: tests/P2321R2_views_zip_transform:077 (98 of 116)
PASS: std :: tests/P3349R1_contiguous_iterators_to_pointers:28 (62 of 64)
PASS: std :: tests/VSO_0000000_vector_algorithms:037 (112 of 138)
PASS: std :: tests/VSO_0961751_hash_range_erase:25 (37 of 46)I am still running the libc++ tests (for reference; will update once i have the results): |
Then I suspect it is a spurious CI failure. Observe that only one shard has failed tests, whereas I'd expect them to be distributed across 10 of them. Let's assume it passed, and you only owe CLA here. |
OK, awesome, thank you! Yes, I am working with the OSPO to get the paperwork for the CLA finished at my company internally. We expect that process to be finished tomorrow. After that, I will start the process here and make sure the appropriate people take the right follow-up steps to finish it (should not take long). Do we need to run the benchmarks as well? |
I assume that you don't need to write your own benchmark, but it would be good to run existing benchmarks before and after your changes, for Observe that out of all #5479 PRs I only added a new benchmark in #5591, to see if adding odd size support really helps I'm not a maintainer though, I only contributed a part of the vectorization here. |
|
I reran the failed shard and it passed; it looked like the machine got corrupted and it wasn't our test suite's fault (much less yours). I am not terribly concerned about running benchmarks for a fix like this; the benchmarks are useful when making tuning decisions. I am severely overloaded at the moment (half of my time is going to non-STL work - not my choice, sorry!) but your PR is on my radar and I'll review it when I get a chance. The only feedback I have right now is to remove the bug number from the PR title, where it provides no value. (The PR description saying |
find vectorization with LLVMfind vectorization with LLVM
Great! Thanks for the input
No worries, same for me and the project I'm working on
Done ✅ |
|
@microsoft-github-policy-service agree company="Alliander" |
Fixes #6294
Here's my attempt at excluding odd-sized and large-sized structs from the vectorization in
xutility.Design approach
I have tried to follow all the conventions used elsewhere as much as possible, in particular:
_Equal_memcmp_is_safeas the currentmainimplementation of_Vector_alg_in_find_is_safewas not tested in the first place. I still opted to add a couple more tests than for_Vector_alg_in_search_is_safeto at the very least show that there's vectorization for MSVC and not only for Clang_Vector_alg_in_search_is_safealready had a similar check as what was needed here, I re-used that one (had to move it outside the#ifdefstatement, though)_VECTORIZED_FIND,static_assert(false, "unexpected size");,_vectorized(and_Nx <= 8 && (_Nx & (_Nx - 1)) == 0to see if there were other cases that I missed. I believe (and hope) I got them all (at least inalgorithmandxutility).MINMAX-like things, because they are independently explicitly guarded where the vectorized versions are used.Additional remarks/open questions
main, so those are not an issue with my branch.<xutility>: std::find no longer compiles with structs with defaulted equality comparison with Clang-CL #6294 ?For performance-sensitive code (e.g. containers and algorithms) you may wish to write and/or run benchmarks, and the STL team will likely run any benchmarks we do have in our PR process.)Despite my questions, I think (and hope) that my contribution is in a good state anyways.