Skip to content

<algorithm>: Fix conditions for vectorization on trivial assignability #5528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

frederick-vs-ja
Copy link
Contributor

It's shown in WG21-P3279R0 that is_trivially_(copy_,move_)assignable is sometimes not it self a suitable condition for vectorization, because a derived class can be "trivially-assigned" via a trivial assignment operator of its base class.

This PR strengthens the conditions by checking the return type of the assignment. I.e. the corresponding conditions are now that

  • T is trivially assignable from the source, and
  • the assignment returns T&, which means that when T is a class, a defaulted and trivial T::operator= is selected, and
  • other conditions.

Pedantically, a defaulted and trivial T::operator= possibly skips a subobject due to aforementioned weird derived classes. But currently, all mainstream compilers don't think so (see LLVM-37038, "reject-valid" demo, and "accept-invalid" demo). So I think the condition is safe despite not being theoretically sufficient.

Also, (ranges::)reverse_copy should rely on trivial assignability, and we shouldn't vectorize it for element type pair<T&, U&> just because trivially copyability. Actually, being trivially copyable doesn't mean any expected operation is publicly available and trivial.

Affected algorithms:

  • (ranges::)copy,
  • (ranges::)copy_n,
  • (ranges::)copy_backward,
  • (ranges::)move,
  • (ranges::)move_backward,
  • (ranges::)reverse_copy.

Fixes #4686.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner May 19, 2025 23:17
@github-project-automation github-project-automation bot moved this to Initial Review in STL Code Reviews May 19, 2025
@StephanTLavavej StephanTLavavej added the bug Something isn't working label May 19, 2025
@StephanTLavavej StephanTLavavej moved this from Initial Review to Work In Progress in STL Code Reviews May 19, 2025
@StephanTLavavej StephanTLavavej moved this from Work In Progress to Initial Review in STL Code Reviews May 20, 2025
@StephanTLavavej StephanTLavavej self-assigned this May 20, 2025
@StephanTLavavej
Copy link
Member

Thanks! 😻 I pushed test cleanups/simplifications.

@StephanTLavavej StephanTLavavej removed their assignment May 21, 2025
@StephanTLavavej StephanTLavavej moved this from Initial Review to Ready To Merge in STL Code Reviews May 21, 2025
@StephanTLavavej StephanTLavavej moved this from Ready To Merge to Merging in STL Code Reviews May 22, 2025
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

…n when calling ranges algorithms with PMD projections".
@StephanTLavavej
Copy link
Member

I've pushed a commit to perma-workaround VSO-1664341 "/clr C++20 System.NullReferenceException when calling ranges algorithms with PMD projections". The ranges::equal calls aren't the point of the test, and now that we have the extracted lambda, getting fancy with PMD projections is more verbose.

@StephanTLavavej StephanTLavavej merged commit 41ec195 into microsoft:main May 22, 2025
40 checks passed
@github-project-automation github-project-automation bot moved this from Merging to Done in STL Code Reviews May 22, 2025
@StephanTLavavej
Copy link
Member

Thanks for figuring out how to fix this "trivial" bug! 🐞 🧠 🛠️

@frederick-vs-ja frederick-vs-ja deleted the vectorize-trivially-assignable branch May 22, 2025 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

<algorithm>: reverse_copy is mistakenly vectorized for pair<int&, int&> on 32-bit targets
2 participants