Use popcount from C++20 if available #551

axic · 2020-09-24T14:26:52Z

Part of #543.

lib/fizzy/execute.cpp

chfast · 2020-09-30T13:16:52Z

lib/fizzy/cxx20/numeric.hpp

+template <class T>
+constexpr int popcount(T x) noexcept
+{
+    return std::popcount.as<T>(x);


What is this?

The using statement didn't seem to work for functions. We still want to "copy" it from std to fizzy.

using does not work because functions are constant objects, not types. This is good approach. But the .as<T> is wrong. And because of we cannot test it now, maybe add static_assert() for now?

static_assert for what? The two types we support/test?

In that case it may be better not to use a template just the two specialisations.

I propose to this for now:

#if __cpp_lib_bitops static_assert(false, "Add alias to std::popcount"); #endif

As for function alias, only template function is generic function alias. There is also an option to use constexpr, but not for overloaded functions: https://stackoverflow.com/questions/9864125/c11-how-to-alias-a-function/49819511#49819511.

Why don't we add a test run for C++20? There is just no point adding these things otherwise.

None of the C++ stdlib implementations have <bit> support.

lib/fizzy/cxx20/numeric.hpp

chfast · 2020-09-30T13:22:21Z

lib/fizzy/cxx20/numeric.hpp

+
+#pragma once
+
+#if __cplusplus > 201703L


This will not work, because you may be building in C++20, but without <bit> implemented.
Use __cpp_lib_bitops.

Should use __cpp_lib_span for span too. And likewise #542 (comment) could be merged.

Correct, although the std::bit_cast is similarly untestable.

lib/fizzy/cxx20/numeric.hpp

gumb0 · 2020-09-30T14:25:49Z

Looks good, needs some unit tests.

axic · 2020-09-30T20:22:46Z

Looks good, needs some unit tests.

We do have tests for them via the opcodes, but you are right probably better to have unit tests for any of these wrappers.

codecov · 2020-09-30T20:45:23Z

Codecov Report

Merging #551 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #551   +/-   ##
=======================================
  Coverage   98.25%   98.26%           
=======================================
  Files          63       63           
  Lines        9243     9263   +20     
=======================================
+ Hits         9082     9102   +20     
  Misses        161      161

axic · 2020-10-05T15:58:09Z

lib/fizzy/cxx20/bit.hpp

+    return __builtin_popcount(x);
+}
+
+constexpr int popcount(uint64_t x) noexcept


These should be inline, not consexpr as __builtin_popcount isn't constexpr.

Most builtins are constexpr. https://godbolt.org/z/Pnz4ob.

Cool, the gcc documentation had no mention of either.

Clang actually marks them constexpr: https://clang.llvm.org/docs/LanguageExtensions.html#intrinsics-support-within-constant-expressions

But GCC does not clarify it: https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html

axic · 2020-10-08T17:41:44Z

@gumb0 added tests if you want to check

gumb0

Looks good to me, needs squashing.

axic · 2020-10-09T12:21:52Z

Will squash, but this is still blocked by clang11 for having a way to tests the C++20 implementation.

axic · 2020-10-09T12:24:17Z

test/unittests/CMakeLists.txt

    capi_test.cpp
    constexpr_vector_test.cpp
+    cxx20_bit_test.cpp
+    cxx20_span_test.cpp


@gumb0 @chfast what do you think about this renaming?

I think it's fine.

axic · 2020-10-13T18:56:32Z

lib/fizzy/execute.cpp

@@ -296,7 +296,7 @@ inline uint64_t ctz64(uint64_t value) noexcept

 inline uint64_t popcnt64(uint64_t value) noexcept


Should make these constexpr then since we established the intrinsics are.

chfast · 2020-10-13T19:16:58Z

lib/fizzy/cxx20/bit.hpp

+
+#else
+
+#include <cstdint>


This is also needed for C++20 variant.

Right. Will fix. Though it seems all the places this is used at already includes cstdint. See the f commit which has a warning to see if the code path is compiled (and it is).

Just include it on the top of the file.

chfast · 2020-10-13T20:17:18Z

lib/fizzy/cxx20/bit.hpp

+
+#else
+
+#include <cstdint>


Just include it on the top of the file.

axic commented Sep 24, 2020

View reviewed changes

lib/fizzy/execute.cpp Outdated Show resolved Hide resolved

axic force-pushed the popcount branch from bd90dd9 to a324ee3 Compare September 24, 2020 22:23

axic force-pushed the popcount branch 2 times, most recently from 27d4d43 to 88582bd Compare September 30, 2020 13:00

axic marked this pull request as ready for review September 30, 2020 13:00

axic requested review from chfast and gumb0 and removed request for chfast and gumb0 September 30, 2020 13:01

chfast requested changes Sep 30, 2020

View reviewed changes

chfast mentioned this pull request Sep 30, 2020

Make use of C++20 bit manipulation features #543

Closed

axic force-pushed the popcount branch from 88582bd to 0b982ea Compare September 30, 2020 14:19

axic force-pushed the popcount branch from 0b982ea to 9da9af6 Compare September 30, 2020 20:22

axic marked this pull request as draft September 30, 2020 20:23

axic force-pushed the popcount branch from 9da9af6 to d860871 Compare October 2, 2020 11:24

axic commented Oct 5, 2020

View reviewed changes

axic force-pushed the popcount branch 2 times, most recently from a4c8353 to 45dc469 Compare October 8, 2020 17:41

axic force-pushed the popcount branch from 45dc469 to 5ed7e90 Compare October 8, 2020 20:18

gumb0 approved these changes Oct 9, 2020

View reviewed changes

axic commented Oct 9, 2020

View reviewed changes

axic force-pushed the popcount branch from 5ed7e90 to ba568d8 Compare October 13, 2020 18:37

axic marked this pull request as ready for review October 13, 2020 18:52

axic requested review from chfast and gumb0 October 13, 2020 18:52

axic commented Oct 13, 2020

View reviewed changes

chfast reviewed Oct 13, 2020

View reviewed changes

axic force-pushed the popcount branch from 105e2f4 to 4203dac Compare October 13, 2020 20:01

axic requested a review from chfast October 13, 2020 20:01

chfast requested changes Oct 13, 2020

View reviewed changes

lib/fizzy/cxx20/bit.hpp Outdated

#else

#include <cstdint>

Copy link

Collaborator

chfast Oct 13, 2020

Choose a reason for hiding this comment

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

Just include it on the top of the file.

axic added 3 commits October 13, 2020 21:22

Add cxx20 support for std::popcount

4e88715

test: rename C++20 tests

6c06ce6

test: add unit tests for popcount

f1a95e4

axic force-pushed the popcount branch from 4203dac to f1a95e4 Compare October 13, 2020 20:22

axic requested a review from chfast October 13, 2020 20:22

chfast approved these changes Oct 13, 2020

View reviewed changes

axic merged commit 28e8272 into master Oct 13, 2020

axic deleted the popcount branch October 13, 2020 20:45

axic mentioned this pull request Oct 13, 2020

Cpp20 bit manipulations #570

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use popcount from C++20 if available #551

Use popcount from C++20 if available #551

axic commented Sep 24, 2020

chfast Sep 30, 2020

axic Sep 30, 2020

chfast Sep 30, 2020

axic Sep 30, 2020

axic Sep 30, 2020

chfast Sep 30, 2020

axic Sep 30, 2020

chfast Sep 30, 2020

chfast Sep 30, 2020

axic Sep 30, 2020

chfast Sep 30, 2020

gumb0 commented Sep 30, 2020

axic commented Sep 30, 2020

codecov bot commented Sep 30, 2020 •

edited

Loading

axic Oct 5, 2020

chfast Oct 5, 2020

axic Oct 5, 2020

axic Oct 13, 2020

axic commented Oct 8, 2020

gumb0 left a comment

axic commented Oct 9, 2020

axic Oct 9, 2020

gumb0 Oct 9, 2020

axic Oct 13, 2020

chfast Oct 13, 2020

axic Oct 13, 2020

chfast Oct 13, 2020

axic Oct 13, 2020

chfast Oct 13, 2020

		@@ -296,7 +296,7 @@ inline uint64_t ctz64(uint64_t value) noexcept

		inline uint64_t popcnt64(uint64_t value) noexcept

Use popcount from C++20 if available #551

Use popcount from C++20 if available #551

Conversation

axic commented Sep 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Sep 30, 2020

axic commented Sep 30, 2020

codecov bot commented Sep 30, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Oct 8, 2020

gumb0 left a comment

Choose a reason for hiding this comment

axic commented Oct 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 30, 2020 •

edited

Loading