This is an archive of the discontinued LLVM Phabricator instance.

[libc++] P1645 constexpr for <numeric>
ClosedPublic

Authored by Mordante on Nov 1 2020, 11:21 AM.

Details

Summary

Implements P1645: constexpr for <numeric> algorithms

Diff Detail

Event Timeline

Mordante created this revision.Nov 1 2020, 11:21 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 1 2020, 11:21 AM
Herald added a subscriber: arichardson. · View Herald Transcript
Mordante requested review of this revision.Nov 1 2020, 11:21 AM
ldionne requested changes to this revision.Nov 3 2020, 5:42 AM

Or do you prefer small patches per function? (In that case I'll to update the status since it now expects one patch.)

One patch for all is fine. But it's a good idea to iterate on this one function for now until we are good to go, and then you can do the other ones. This will avoid back and forth.

libcxx/include/numeric
20

Please add // constexpr since C++20 to note that the API is only constexpr in C++20.

libcxx/test/std/numerics/numeric.ops/accumulate/accumulate.pass.cpp
54

The tests for runtime and constexpr should be the same. Can you please take a look at libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/destroy_n.pass.cpp to see how we do it elsewhere?

This revision now requires changes to proceed.Nov 3 2020, 5:42 AM
Mordante marked 2 inline comments as done.Nov 3 2020, 10:39 AM

Or do you prefer small patches per function? (In that case I'll to update the status since it now expects one patch.)

One patch for all is fine. But it's a good idea to iterate on this one function for now until we are good to go, and then you can do the other ones. This will avoid back and forth.

Thanks for the feedback. Then I'll make one patch for the paper once the first function is approved.

libcxx/test/std/numerics/numeric.ops/accumulate/accumulate.pass.cpp
54

That's a nice approach and indeed looks better than my approach.

Mordante updated this revision to Diff 302620.Nov 3 2020, 10:45 AM
Mordante marked an inline comment as done.

Addresses the review comments. Adds a comment and improves the unit tests.

I had another look at the header and noticed something odd for all overloads of inclusive_scan and transform_inclusive_scan and their signatures are odd

template <class _InputIterator, class _OutputIterator, class _Tp, class _BinaryOp>
_OutputIterator inclusive_scan(_InputIterator __first, _InputIterator __last,
                               _OutputIterator __result, _BinaryOp __b,  _Tp __init)

Instead of the expected

template <class _InputIterator, class _OutputIterator, class _Tp, class _BinaryOp>
inline _LIBCPP_INLINE_VISIBILITY
_OutputIterator
inclusive_scan(_InputIterator __first, _InputIterator __last,
               _OutputIterator __result, _BinaryOp __b,  _Tp __init)

Is there a reason why their signatures differ or is it an oversight?
Shall I fix them in this patch?

libcxx/test/std/numerics/numeric.ops/accumulate/accumulate.pass.cpp
10–11

I reverted the original change and redid the changes. I just noticed I forgot to re-add the // Became constexpr in C++20 comment here.

ldionne requested changes to this revision.Nov 10 2020, 6:39 AM

I had another look at the header and noticed something odd for all overloads of inclusive_scan and transform_inclusive_scan and their signatures are odd

template <class _InputIterator, class _OutputIterator, class _Tp, class _BinaryOp>
_OutputIterator inclusive_scan(_InputIterator __first, _InputIterator __last,
                               _OutputIterator __result, _BinaryOp __b,  _Tp __init)

Instead of the expected

template <class _InputIterator, class _OutputIterator, class _Tp, class _BinaryOp>
inline _LIBCPP_INLINE_VISIBILITY
_OutputIterator
inclusive_scan(_InputIterator __first, _InputIterator __last,
               _OutputIterator __result, _BinaryOp __b,  _Tp __init)

Is there a reason why their signatures differ or is it an oversight?
Shall I fix them in this patch?

I believe those are oversights, and the blame seems to confirm this. Good catch, and please fix them in this patch. You don't need inline though, just _LIBCPP_INLINE_VISIBILITY. These functions are already inline (i.e. linkonce-ODR) because they are templates.

On a different note, I would like to mention that I really like your approach of making sure everything's fine with a single function, and then going for the whole thing. I feel like this is a productive way to proceed, and I appreciate that.

Your approach LGTM, feel free to go ahead with the full thing!

This revision now requires changes to proceed.Nov 10 2020, 6:39 AM
Mordante updated this revision to Diff 306144.Nov 18 2020, 10:24 AM

Added an updated test for libcxx/test/std/numerics/numeric.ops/exclusive.scan/exclusive_scan.pass.cpp.

Since this tests uses std::vector which can't be used as constexpr in libc++ at the moment I took a different approach. Several other tests have the same issue. @ldionne can you have a look only at this test before I proceed?

Is there a reason why their signatures differ or is it an oversight?
Shall I fix them in this patch?

I believe those are oversights, and the blame seems to confirm this. Good catch, and please fix them in this patch. You don't need inline though, just _LIBCPP_INLINE_VISIBILITY. These functions are already inline (i.e. linkonce-ODR) because they are templates.

Thanks will do.

On a different note, I would like to mention that I really like your approach of making sure everything's fine with a single function, and then going for the whole thing. I feel like this is a productive way to proceed, and I appreciate that.

Thanks. I also prefer this way since it saves everybody time!

ldionne requested changes to this revision.Nov 18 2020, 11:35 AM
ldionne added inline comments.
libcxx/test/std/numerics/numeric.ops/exclusive.scan/exclusive_scan.pass.cpp
40

LGTM, but could you add a FIXME to remind us to go back and fix those once constexpr std::vector is finished?

If you keep the FIXME exactly the same in all places where you use this pattern, we'll be able to search for it and easily replace all locations where we used the workaround.

This revision now requires changes to proceed.Nov 18 2020, 11:35 AM
Mordante updated this revision to Diff 307103.Nov 23 2020, 9:51 AM
Mordante retitled this revision from [RFC] [libc++] P1645 constexpr for <numeric> to [libc++] P1645 constexpr for <numeric>.

This should complete the implementation of P1645
Additional changes

  • Since constexpr vector isn't available some test have a workaround to be tested without this feature. The changes are marked with a fixme to be removed once the support is available.
  • Enables additional unit tests in libcxx/test/std/numerics/numeric.ops/inclusive.scan/inclusive_scan_op.pass.cpp. These were disabled when added, but that seems an oversight.
  • Changes the changed numeric functions to all use _LIBCPP_INLINE_VISIBILITY and removed the unneeded inline.
ldionne accepted this revision.Nov 23 2020, 2:19 PM
ldionne set the repository for this revision to rG LLVM Github Monorepo.

This LGTM, but I'd really like to see CI running. I added the LLVM monorepo to the revision -- can you try rebasing onto master just to see if it'll trigger CI?

This revision is now accepted and ready to land.Nov 23 2020, 2:20 PM

Thanks for the review.
I've switched to arc, so that should solve the CI issues.

Mordante updated this revision to Diff 307376.Nov 24 2020, 8:45 AM
Mordante edited the summary of this revision. (Show Details)

Rebase to trigger CI.

Mordante updated this revision to Diff 307558.Nov 25 2020, 2:31 AM

Rebase to trigger CI.

This revision was automatically updated to reflect the committed changes.
Mordante reopened this revision.Nov 25 2020, 4:48 AM

Reopened since I reverted to commit. It fails on several build bots using LLVM 8.

This revision is now accepted and ready to land.Nov 25 2020, 4:48 AM
Mordante updated this revision to Diff 308163.Nov 28 2020, 6:48 AM

Disable the unit tests on clang-8, they fail and we don't support these old compilers for libc++.
Switch to the new status page
Adjusted the unit tests modified in D61170 to work in constexpr evaluation. Since constexpr string isn't supported yet the std::string tests are disabled in constant evaluation context.

This revision was automatically updated to reflect the committed changes.