Vectorize swap and ranges::swap for arrays#2689
Merged
StephanTLavavej merged 6 commits intomicrosoft:mainfrom May 1, 2022
Merged
Vectorize swap and ranges::swap for arrays#2689StephanTLavavej merged 6 commits intomicrosoft:mainfrom
swap and ranges::swap for arrays#2689StephanTLavavej merged 6 commits intomicrosoft:mainfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
CaseyCarter
reviewed
Apr 29, 2022
<utility>: Use vectorized swapping in swap(T (&)[N], T (&)[N])swap and ranges::swap for arrays
Member
Author
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
CaseyCarter
approved these changes
May 1, 2022
StephanTLavavej
added a commit
to StephanTLavavej/STL
that referenced
this pull request
May 3, 2022
This reverts commit 39332e1.
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.
Fixes #2683.
The (minor) main difficulty here is that the vectorized machinery was defined in
<xutility>, butswap()for arrays is defined in its parent header<utility>, andranges::swapis defined in<concepts>. To solve this, I'm moving the definition of_USE_STD_VECTOR_ALGORITHMSup to<type_traits>(unchanged), then extracting only the declaration of__std_swap_ranges_trivially_swappable_noalias(), with a comment directing the reader to<xutility>. (Any merge conflicts with vectorization PRs in flight should be trivial to resolve.)Because
<utility>and<concepts>are "core" headers, I'm also adding a comment that if users want to use them without linking to the STL, they might need to disable our use of vectorized algorithms.Since we've already extensively tested this implementation, I'm adding a small amount of incremental test coverage. This covers various element sizes and array lengths (from 1 element of 1 byte, to 512 elements of 8 bytes).
Small cleanup in
<utility>: Because the control flow is becoming more complicated, I'm changing theif (&_Left != &_Right)self-swap guard to anif (_First1 == _First2)early return. This uses the named pointers we already need, instead of taking the addresses of the arrays.In
<concepts>, I'm usingconst autoto decay the arrays; I felt that this looked simpler than saying_Ty1here even though we know that it's the same as_Ty2. Then, we need to defend__std_swap_ranges_trivially_swappable_noaliaswith a self-swap check; I believe that it should be within the "we're eligible for vectorization" preprocessor andif constexprchecks (because the condition is rare), but that it didn't need to be within the "we're at runtime" codepath (it's okay for constant evaluation to perform the self-swap check).