Repository navigation
Conversation
xreducer_stepper::aggregate_impl had two overloads. The one used without keep_dims walks the reduced axes, m_axes[0] to m_axes.back(). The keep_dims one walked every dimension of the expression, looked each one up in m_axes with std::find, and recursed through the dimensions that are not reduced without doing anything there. Both do the same steps on the underlying stepper: keep_dims only changes the shape of the result, which the stepper does not use here. The extra size != 0 test of the keep_dims overload is also redundant, since aggregate() returns the initial value for empty expressions before calling aggregate_impl. The keep_dims overload is removed. GCC 13 at -O3 inlined that recursion and warned about m_e.shape()[dim] for dim == dimension() (-Warray-bounds), a path that the axis validation in the constructor rules out. The warning goes away with the overload. The new test compares lazy keep_dims sums with immediate ones for every set of axes of a 4-D array, in row and column major order and with an initial value.
Contributor
Merging this PR will regress 1 benchmark1 warning
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing |
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.
Checklist
Description
xreducer_stepper::aggregate_implhad a second overload forkeep_dims. It walked every dimension of the expression, looked each one up inm_axeswithstd::find, and recursed through the dimensions that are not reduced without doing anything there. The other overload walksm_axesdirectly.Both apply the same steps to the underlying stepper, since
keep_dimsonly changes the shape of the result. The extrasize != 0test of thekeep_dimsoverload is redundant too:aggregate()already returns the initial value for empty expressions. This PR removes thekeep_dimsoverload.GCC 13 at
-O3inlined that recursion and warned aboutm_e.shape()[dim]withdim == dimension()(-Warray-bounds, 18 occurrences intest_xmath). The axis checks in the constructor rule that path out, and the warning goes away with the overload.xreducer.keep_dims_lazy_all_axescompares lazykeep_dimssums with immediate ones for every set of axes of a 4-D array, in row- and column-major order and with an initial value. It passes before and after the change, also under AddressSanitizer and UBSan.