Skip to content
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

<valarray>: Consider replacing size() calls with _Mysize #3915

Closed
StephanTLavavej opened this issue Jul 31, 2023 · 1 comment · Fixed by #3968
Closed

<valarray>: Consider replacing size() calls with _Mysize #3915

StephanTLavavej opened this issue Jul 31, 2023 · 1 comment · Fixed by #3968
Labels
fixed Something works now, yay! performance Must go faster

Comments

@StephanTLavavej
Copy link
Member

Followup issue after discussion in #3911, to improve debug codegen for a minor editing cost.

As @AlexGuteniev mentioned in #3911 (comment) , <valarray> is currently extremely careful to extract the size to a local variable to avoid aliasing, which is important for vectorizing its following loops. When replacing calls of size() with direct access to _Mysize, the local variables should be preserved. We may even want to add comments like // avoid aliasing as a small reminder about this somewhat unusual pattern.

@AlexGuteniev
Copy link
Contributor

Fun facts:

  • Capturing size() was suggested by @miscco for a reason unrelated to vectorization. Back then I had less experience with vectorization and didn't see it also helps vectorizing Valarray loops expansion #1165 (comment)
  • There's still a place where we don't capture size - it is _Tidy_deallocate(). Capturing _Mysize there wouldn't normally help vectorization, as the types that are vectorized are normally trivially destructible. For debug codegen if constexpr skipping that part may help

STL/stl/inc/valarray

Lines 558 to 560 in 0fe653a

for (size_t _Idx = 0; _Idx < _Mysize; ++_Idx) {
_Destroy_in_place(_Myptr[_Idx]);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed Something works now, yay! performance Must go faster
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants