New Literals Utils #80

jamierpond · 2024-04-16T23:46:08Z

moved this into a new PR for clarity. Literals can now be written like so:

static_assert(SWAR{Literals<32, u64>, {2, 1}}.value() == 0x00000002'00000001);
static_assert(SWAR{Literals<32, u64>, {1, 2}}.value() == 0x00000001'00000002);
static_assert(SWAR{Literals<16, u64>, {4, 3, 2, 1}}.value() == 0x0004'0003'0002'0001);
static_assert(SWAR{Literals<16, u64>, {1, 2, 3, 4}}.value() == 0x0001'0002'0003'0004);

thecppzoo · 2024-04-17T21:21:38Z

inc/zoo/swar/SWAR.h

+template <int NBits, typename T> struct SWAR;
+
+template <int NumBits, typename BaseType> struct Literals_t {
+  constexpr static void (SWAR<NumBits, BaseType>::*value)() = nullptr;


This is not in use, remove

thecppzoo · 2024-04-17T21:25:25Z

inc/zoo/swar/SWAR.h

+        m_v = result;
+    }
+
+    constexpr static T MaxUnsignedLaneValue = ~(((~T{0}) << (NBits - 1)) << 1);


I agree this constant is worth including, but not here, and not with this calculation. See there are a bunch of constants in SWAR, you may, for example, put it at the end of the list like this:

MaxUnsignedLaneValue = LeastSignificantLaneMask;

thecppzoo · 2024-04-17T21:29:41Z

test/swar/BasicOperations.cpp

+static_assert(SWAR<16, u64>::MaxUnsignedLaneValue == 65535);
+static_assert(SWAR<16, u32>::MaxUnsignedLaneValue == 65535);
+static_assert(SWAR<8, u32>::MaxUnsignedLaneValue == 255);
+static_assert(SWAR<4, u32>::MaxUnsignedLaneValue == 15);
+static_assert(SWAR<2, u32>::MaxUnsignedLaneValue == 3);


Decimal is not the natural base for these tests.
I know we ahve a mess of decimals and hex and binary in these tests, but let's stop the mess.
You may create an assign a PR to me to clean this up if you're upset about this request.

I understand why we might want to have just one format, decimal makes the repo a little more approachable and understandable.

test/swar/BasicOperations.cpp

inc/zoo/swar/SWAR.h

jamierpond · 2024-04-19T15:16:39Z

inc/zoo/swar/SWAR.h

+    template <typename U, typename ManipulationFn>
+    constexpr auto loadBaseTypeIntoLanes(const U (&values)[Lanes],
+                                         const ManipulationFn&& manipulation) {
+        auto result = T{0};
+        for (auto value : values) {
+            auto laneValue = manipulation(value);
+            result = (result << NBits) | laneValue;
+        }
+        return result;
+    }


not 100% sure about the names for manipulation etc right now but would be good to get your opinion on this approach.

This might be a reasonable way to do this, but I'd use the appropriate algorithm in #include <algorithm> and I would not take by const-r-value reference something that ought to be a pure forwarding reference, meaning ManipulationFN &&
Make sure you never put const-r-value, either const-l-value when strictly necessary, or forwarding reference whenever possible.

inc/zoo/swar/SWAR.h

test/swar/BasicOperations.cpp

thecppzoo

Improvements all around, but, like in the movie "Whiplash", still not quite my tempo.

Since you have given us the construction from literals, perhaps it is a good idea to do the opposite of having a conversion operator to array of values,
constexpr operator std::array<T, Lanes>();
and a named conversion to_array

thecppzoo · 2024-04-19T16:24:38Z

inc/zoo/swar/SWAR.h

-    }
+    template <std::size_t N>
+    constexpr BooleanSWAR(Literals_t<NBits, T>, const bool (&values)[N])
+       : Base(Literals<NBits, T>, values) { this->m_v << (NBits - 1); }


My typo in the comment, this has no effect, change to <<=

great, works perfectly.

jamierpond · 2024-04-19T16:38:49Z

Improvements all around, but, like in the movie "Whiplash", still not quite my tempo.
Loving the feedback. Thank you!
Since you have given us the construction from literals, perhaps it is a good idea to do the opposite of having a conversion operator to array of values, constexpr operator std::array<T, Lanes>(); and a named conversion to_array
I like the idea of this, will check it out.

thecppzoo · 2024-04-19T17:00:01Z

inc/zoo/swar/SWAR.h

        }
        return result;
    }

    template <typename Arg, std::size_t N, typename = std::enable_if_t<N == Lanes, int>>
    constexpr
    SWAR(Literals_t<NBits, T>, const Arg (&values)[N])
-      : m_v{loadIntoLanes(values, [](auto x) { return x; })} {}
+      : m_v{loadIntoLanes(values)} {}


May you please indent to multiples of four spaces?

thecppzoo

You're free to merge yourself after you're satisfied

thecppzoo · 2024-04-19T17:05:08Z

inc/zoo/swar/SWAR.h

+    constexpr static auto calculateLeastSignificantLaneMask() {
+      const auto m =
+            sizeof(T) * 8 == NBits ? // needed to avoid shifting all bits
+                ~T(0) :
+                ~(~T(0) << NBits);
+      return m;
+    }
    constexpr static inline type


Nah...
This calculation does not deserve a name.
I think since C++17 we can use constexpr lambdas, that would be more appropriate below.
However, I showed you that this calculation is not needed, you're repating, and that's the bad part, the bulk of the work done in other constants.
Better than repeating the work is to use the work already done, primarily because then you have cross-checking that things work, if there's a mistake in the old, the new might discover it, if further nuance is discovered the new might benefit from it directly.

Thanks, should have reverted. This was a debugging attempt. Was getting a red herring compiler message about this variable not being constexpr when trying to implement to_array.

thecppzoo

The latest changes made the PR not mergeable anymore, please review

thecppzoo · 2024-04-19T17:08:05Z

inc/zoo/swar/SWAR.h

+    constexpr std::array<T, Lanes> to_array() const noexcept {
+        std::array<T, Lanes> result;
+        for (int i = 0; i < Lanes; ++i) {
+            auto otherEnd = Lanes - i - 1;
+            result[otherEnd] = at(i);
+        }
+        return result;
+    }
+


This endianness conversion bothers me.
Your code is correct, but there is something about endianness that is creeping up that is bothering me.

Yeah I agree there is something to figure out here. I think you would agree that the to_array should produce arrays with the same endianness as the input like this:

static_assert(SWAR{Literals<4, u8>, {1, 2}}.to_array() == std::array<u8, 2>{1, 2});

Not sure the best thing to suggest here.

Your code is correct. Furthermore, your choice to adopt big endianness for arrays was superior to the alternative, but now we have this flipping of endianness that I don't know what to do about; I'm just voicing the idea, and plan to keep it in the back of my head to see where we go with this.

thecppzoo · 2024-04-19T17:11:07Z

test/CMakeLists.txt

@@ -81,7 +81,7 @@ if(MSVC)
    endif()
 else()
    # Non-MSVC specific configuration (original content)
-    set(CMAKE_CXX_STANDARD 17)
+    set(CMAKE_CXX_STANDARD 23)


Support for compilation under MSVC is on a "best effort" basis, we try, but we can't guarantee we will succeed at all the things. It is acceptable to bump to 23, but please explain why you found the need

This is for the non-MSVC path. This is to enable support for std::array in a constexpr context.

Reverted, we should address in another PR. This one is already hard to keep track of.

jamierpond · 2024-04-19T18:30:49Z

@thecppzoo maybe best to merge where we were happy and add the to_array in a seperate PR if we think that is interesting?

add to_array oops make 23 for now not sure we want to commit to this? remove named function Revert "Add to Array" This reverts commit a7d744d.

thecppzoo · 2024-04-19T18:37:12Z

inc/zoo/swar/SWAR.h

+        MaxUnsignedLaneValue = LeastSignificantLaneMask;
+
+    template <typename U>
+    constexpr auto loadIntoLanes(const U (&values)[Lanes]) const noexcept {


Please rename to from_array. As the symmetrical operation to to_array.
Also, this is a factory, it ought to be static, a class-function, not instance.
Actually, we also need a constructor from std::array.

Apologies for all of the creep, I just think there is a lot of extra value by making SWAR a really ergonomic type as opposed to somewhat ergonomic.

I dislike that C++ forces you to do a lot of boilerplate, I've never figured out how to make tools to eliminate the work to do the boilerplate.

thecppzoo · 2024-04-19T18:49:44Z

My preference with this PR is to not just introduce the support for direct initialization of literals, but complete it with the following:

Making the essential function from_array a part of the public interface (we ought to indicate the semantics is that of big endianness in the comments inline with the code), as a class-function, because it is a factory.
Keep the Literals infrastructure you already made, extend with a constructor from std::array
to_array

Separately, I think we can also add a header, such as SWAR_IO.h where the insertion and extraction operators are implemented. I don't think these things are unrelated: The ergonomics of the literals are highly cohesive with stream insertion and extraction. We need this "yesterday" for debugging and tracing.

Indicate if you'd like to split these features with me or prefer to do them all yourself.

jamierpond · 2024-04-20T00:55:56Z

If you want to take on the std::array stuff and getting that to that could be nice.

From what I understood from playing around with this today we will require C++ >17 in order to use constexpr std::array in static_assert tests, but this may be a limitation of my understanding.

Even with C++23 am seeing failures to compiler on g++.

Scottbruceheart · 2024-06-02T23:34:29Z

inc/zoo/swar/SWAR.h

+    constexpr static auto from_array(const U (&values)[Lanes]) noexcept {
+        auto result = T{0};
+        for (auto value : values) {
+            result = (result << NBits) | value;


blitElement?

I this does not quite blit the element, if it will move in an element, it shifts up what it got and then adds to the least significant lane

inc/zoo/swar/SWAR.h

new literals

295d602

jamierpond requested review from Scottbruceheart and thecppzoo April 16, 2024 23:46

thecppzoo reviewed Apr 17, 2024

View reviewed changes

thecppzoo requested changes Apr 17, 2024

View reviewed changes

jamierpond added 3 commits April 19, 2024 14:03

PR feedback

dbf8115

tidy tests

2327807

tidy reused code for booleanswaer

7c10d29

jamierpond commented Apr 19, 2024

View reviewed changes

update name

0b8b214

thecppzoo reviewed Apr 19, 2024

View reviewed changes

inc/zoo/swar/SWAR.h Outdated Show resolved Hide resolved

thecppzoo reviewed Apr 19, 2024

View reviewed changes

inc/zoo/swar/SWAR.h Outdated Show resolved Hide resolved

thecppzoo reviewed Apr 19, 2024

View reviewed changes

inc/zoo/swar/SWAR.h Outdated Show resolved Hide resolved

thecppzoo reviewed Apr 19, 2024

View reviewed changes

inc/zoo/swar/SWAR.h Outdated Show resolved Hide resolved

thecppzoo reviewed Apr 19, 2024

View reviewed changes

test/swar/BasicOperations.cpp Outdated Show resolved Hide resolved

thecppzoo reviewed Apr 19, 2024

View reviewed changes

test/swar/BasicOperations.cpp Outdated Show resolved Hide resolved

thecppzoo reviewed Apr 19, 2024

View reviewed changes

test/swar/BasicOperations.cpp Show resolved Hide resolved

thecppzoo requested changes Apr 19, 2024

View reviewed changes

jamierpond added 3 commits April 19, 2024 17:21

style

e3a802d

test style

7ed6e88

fix

03edd02

thecppzoo reviewed Apr 19, 2024

View reviewed changes

jamierpond added 3 commits April 19, 2024 17:30

undef util

0648bb1

simplify again

0d0a833

fmt

33183cd

thecppzoo reviewed Apr 19, 2024

View reviewed changes

thecppzoo approved these changes Apr 19, 2024

View reviewed changes

thecppzoo reviewed Apr 19, 2024

View reviewed changes

thecppzoo requested changes Apr 19, 2024

View reviewed changes

thecppzoo reviewed Apr 19, 2024

View reviewed changes

jamierpond force-pushed the jp/new-literals branch from 4a1138c to a7d744d Compare April 19, 2024 18:31

Add to Array

1cba60e

add to_array oops make 23 for now not sure we want to commit to this? remove named function Revert "Add to Array" This reverts commit a7d744d.

jamierpond force-pushed the jp/new-literals branch from a7d744d to 1cba60e Compare April 19, 2024 18:32

thecppzoo reviewed Apr 19, 2024

View reviewed changes

jamierpond added 2 commits April 19, 2024 19:39

from array

8aa4ea5

rm unused

2a383cb

Scottbruceheart reviewed Jun 2, 2024

View reviewed changes

inc/zoo/swar/SWAR.h Show resolved Hide resolved

jamierpond force-pushed the master branch from bbe3a56 to e45cca2 Compare June 4, 2024 09:05

Finalizing PR

1dc2d89

thecppzoo requested review from thecppzoo and Scottbruceheart June 10, 2024 05:47

Strange Windows fail on unmodified code of Variant

aabab90

thecppzoo approved these changes Jun 10, 2024

View reviewed changes

Scottbruceheart merged commit 0e2a15b into master Jun 10, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Literals Utils #80

New Literals Utils #80

jamierpond commented Apr 16, 2024 •

edited

Loading

thecppzoo Apr 17, 2024

thecppzoo Apr 17, 2024

thecppzoo Apr 17, 2024

jamierpond Apr 19, 2024

jamierpond Apr 19, 2024

thecppzoo Apr 19, 2024

thecppzoo left a comment •

edited

Loading

thecppzoo Apr 19, 2024

jamierpond Apr 19, 2024

jamierpond commented Apr 19, 2024

thecppzoo Apr 19, 2024

thecppzoo left a comment

thecppzoo Apr 19, 2024

jamierpond Apr 19, 2024

thecppzoo left a comment

thecppzoo Apr 19, 2024

jamierpond Apr 19, 2024

thecppzoo Apr 19, 2024

thecppzoo Apr 19, 2024

jamierpond Apr 19, 2024

jamierpond Apr 19, 2024

jamierpond commented Apr 19, 2024

thecppzoo Apr 19, 2024 •

edited

Loading

thecppzoo commented Apr 19, 2024

jamierpond commented Apr 20, 2024 •

edited

Loading

Scottbruceheart Jun 2, 2024

thecppzoo Jun 10, 2024

New Literals Utils #80

New Literals Utils #80

Conversation

jamierpond commented Apr 16, 2024 • edited Loading

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

thecppzoo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamierpond commented Apr 19, 2024

Choose a reason for hiding this comment

thecppzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thecppzoo left a 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

jamierpond commented Apr 19, 2024

thecppzoo Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

thecppzoo commented Apr 19, 2024

jamierpond commented Apr 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamierpond commented Apr 16, 2024 •

edited

Loading

thecppzoo left a comment •

edited

Loading

thecppzoo Apr 19, 2024 •

edited

Loading

jamierpond commented Apr 20, 2024 •

edited

Loading