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

<numeric> Implement P1645R1 "constexpr for <numeric> algorithms" #399

Merged
merged 24 commits into from
Jan 9, 2020

Conversation

Neargye
Copy link
Contributor

@Neargye Neargye commented Dec 19, 2019

Description

This is an in-progress implementation of constexpr <numeric>.
Will resolve issue #337.

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

  • Identifiers in product code changes are properly _Ugly as per
    https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
  • The STL builds successfully and all tests have passed (must be manually
    verified by an STL maintainer before automated testing is enabled on GitHub,
    leave this unchecked for initial submission).
  • These changes introduce no known ABI breaks (adding members, renaming
    members, adding virtual functions, changing whether a type is an aggregate
    or trivially copyable, etc.).
  • These changes were written from scratch using only this repository,
    the C++ Working Draft (including any cited standards), other WG21 papers
    (excluding reference implementations outside of proposed standard wording),
    and LWG issues as reference material. If they were derived from a project
    that's already listed in NOTICE.txt, that's fine, but please mention it.
    If they were derived from any other project (including Boost and libc++,
    which are not yet listed in NOTICE.txt), you must mention it here,
    so we can determine whether the license is compatible and what else needs
    to be done.

@Neargye Neargye requested a review from a team as a code owner December 19, 2019 13:10
@Neargye Neargye force-pushed the constexpr_numeric branch 2 times, most recently from 4803767 to 136d230 Compare December 19, 2019 13:45
Copy link
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did implement the same changes earlier this week but didnt manage to push them / write the appropriate tests.

Generally this looks already quite good. All the comments apply to transform_reduce as well.

Also as a side note I would suggest to create smaller commits. Here it might be simple but I would still split them up into logical parts (My branch history looks like this)

[numeric] make transform_reduce constexpr
[numeric] make reduce constexpr
[numeric] make iota constexpr
[numeric] make adjacent_difference constexpr
[numeric] make transform_inclusive_scan constexpr
[numeric] make transform_exclusive_scan constexpr
[numeric] make inclusive_scan constexpr
[numeric] make exclusive_scan constexpr
[numeric] make partial_sum constexpr
[numeric] make inner_product constexpr
[numeric] make accumulate constexpr
[xutility] make _Idl_distance constexpr

stl/inc/numeric Outdated Show resolved Hide resolved
stl/inc/numeric Outdated Show resolved Hide resolved
stl/inc/numeric Outdated Show resolved Hide resolved
stl/inc/numeric Outdated Show resolved Hide resolved
stl/inc/numeric Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
@Neargye Neargye changed the title <numeric> Implement P1645R1- WIP <numeric> Implement P1645R1 Dec 20, 2019
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
@BillyONeal BillyONeal self-assigned this Jan 8, 2020
@SuperWig
Copy link
Contributor

SuperWig commented Jan 8, 2020

Shouldn't the is_constant_evaluated comments have ()'s?

stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/xutility Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/numeric Show resolved Hide resolved
stl/inc/numeric Show resolved Hide resolved
@BillyONeal BillyONeal changed the title <numeric> Implement P1645R1 <numeric> Implement P1645R1 "constexpr for <numeric> algorithms" Jan 8, 2020
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Show resolved Hide resolved
@BillyONeal
Copy link
Member

Thanks for your contribution @Neargye !

@BillyONeal BillyONeal merged commit 7ad0d63 into microsoft:master Jan 9, 2020
#endif // __cpp_lib_is_constant_evaluated
{
if constexpr (_Plus_on_arithmetic_ranges_reduction_v<_Unwrapped_t<const _InIt&>, _Ty, _BinOp>) {
(void) _Reduce_op; // TRANSITION, VSO-486357
Copy link
Contributor

@miscco miscco Jan 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I am super late to the party, but wouldnt it be possible to move the is_constant_evaluated branch in here, so that we get the throughput improvement from the if constexpr

    if constexpr (_Plus_on_arithmetic_ranges_reduction_v<_Unwrapped_t<const _InIt&>, _Ty, _BinOp>) {
#ifdef __cpp_lib_is_constant_evaluated
        // TRANSITION, DevCom-878972
        if (_STD is_constant_evaluated()) {
            for (; _UFirst != _ULast; ++_UFirst) {
                _Val = _Reduce_op(_STD move(_Val), *_UFirst); // Requirement missing from N4713
            }
            return _Val;
        }
#endif // __cpp_lib_is_constant_evaluated
        (void) _Reduce_op; // TRANSITION, VSO-486357
        return _Reduce_plus_arithmetic_ranges(_UFirst, _ULast, _Val);
    } else {
        for (; _UFirst != _ULast; ++_UFirst) {
            _Val = _Reduce_op(_STD move(_Val), *_UFirst); // Requirement missing from N4713
        }
        return _Val;
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That still has the loop on both branches of the if constexpr, so I don't think it helps.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I've filed #414 to keep track of this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we cannot get rid of the loop in the if constexpr branch as that is the is_constant_evaluated fallback, but this gets rid of the duplication of the loop in the non if constexpr case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm that's fair.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P1645R1 constexpr For <numeric> Algorithms
6 participants