Implement lane-wise modulo. #86

jamierpond · 2024-05-26T07:54:26Z

No safety about the subtract one, but shouldn't be an issue if your values are in range.

Depends on the "ergonomics" PR, which should merge first, and then the "base" of this PR should be moved to master.

add to_array oops make 23 for now not sure we want to commit to this? remove named function

Reverting mistake, this is not a `struct` definition but a variable!

thecppzoo · 2024-05-26T20:37:24Z

inc/zoo/swar/math.h

+#include <cstddef>
+#include <cstdint>
+
+namespace zoo::math {


You know, should you have used maths, I would have let it slide. But since you didn't, if you try to use the British maths I will reject the renaming on the basis that there is no need to make a change like that
;-)

I wouldn't have let 'maths' slide.

thecppzoo · 2024-05-26T20:38:31Z

inc/zoo/swar/math.h

+template <typename IntegerType = size_t>
+constexpr static
+std::enable_if_t<std::is_integral_v<IntegerType>, bool>
+is_power_of_two(IntegerType x) noexcept {


Why snake_case instead of the normal camelCase and DromedaryCamelCase customary in the rest of zoo for these things?

habit, let's move to camelCase to be consistent. will start email thread about case to ask if you have strong preference to avoid polluting PR with unrelated discussion.

thecppzoo · 2024-05-26T20:40:04Z

inc/zoo/swar/math.h

+#pragma once
+#include "SWAR.h"
+#include <cstddef>
+#include <cstdint>


SWAR.h includes this, or its equivalent, stdint.h, I see no point in things like std::uint64_t instead of just uint64_t

thecppzoo · 2024-05-26T20:41:19Z

inc/zoo/swar/math.h

+constexpr static
+std::enable_if_t<std::is_integral_v<IntegerType>, bool>
+is_power_of_two(IntegerType x) noexcept {
+  return x > 0 && (x & (x - 1)) == 0;


Indentation.
Why the X > 0 instead of the customary X or bool(X)?

thecppzoo · 2024-05-27T01:55:24Z

test/swar/BasicOperations.cpp

+static_assert(equals(S{S::Literal,      {1, 2, 3, 4}},
+                     S{S::Literal,      {1, 2, 3, 4}}).value()
+                     == BS{BS::Literal, {T, T, T, T}}.value());
+
+static_assert(equals(S{S::Literal,      {1, 2, 3, 4}},
+                     S{S::Literal,      {5, 6, 7, 8}}).value()
+                     == BS{BS::Literal, {F, F, F, F}}.value());
+
+static_assert(equals(S{S::Literal,      {1, 2, 3, 4}},
+                     S{S::Literal,      {5, 2, 7, 4}}).value()
+                     == BS{BS::Literal, {F, T, F, T}}.value());


NO ACCIDENTAL INDENTATION LEVELS

this is not accidental, but just not intentionally formatted yet. didn't feel super obvious to me what this should look like.

thecppzoo · 2024-05-27T01:57:52Z

I have repeatedly expressed that accidental indentation levels are contrary to the principles of how the sources ought to express more detail.
Besides, they have the practical effect of shifting the important part of the source code to the right, and an incentive to damage the code with wider lines.

thecppzoo · 2024-05-27T02:43:18Z

inc/zoo/swar/SWAR.h

@@ -63,6 +66,7 @@ constexpr std::make_unsigned_t<T> lsbIndex(T v) noexcept {
 template<int NBits_, typename T = uint64_t>
 struct SWAR {
    using type = std::make_unsigned_t<T>;
+    using Boolean = BooleanSWAR<NBits_, T>;


I don't like this forward reference.
It is not that intuition tells me that MSVC won't like this and give us subtle compilation problems because of MSVCs bugs, but something deeper. I don't think that the abstraction of BooleanSWAR is in any way something that SWAR ought to advertise. There's something fishy here that I can not express well.

Yeah I wasn't sure about this either... wanted to see what you thought about this.

thecppzoo · 2024-05-27T02:45:35Z

inc/zoo/swar/math.h

+static_assert(moduloPowerOfTwo<4>(0) == 0);
+static_assert(moduloPowerOfTwo<8>(9) == 1);
+static_assert(moduloPowerOfTwo<4096>(4097) == 1);


I know we have some pollution like this in our headers. It is pollution we haven't bothered to clean. Please move these legitimate compile time tests to their "translation unit" out of the header.

Good callout, thanks.

thecppzoo · 2024-05-27T02:48:52Z

inc/zoo/swar/math.h

+constexpr static auto isPowerOfTwo(S x) noexcept {
+    constexpr auto NBits = S::NBits;
+    using T = typename S::type;
+    auto greater_than_0 = greaterEqual_MSB_off(x, S{0});


Does this work if the MSB is not off?
I think that, for example, a SWAR of 4 bits per lane with one lane containing the MSB, like for example 8, will fail.
I'm sorry that the _MSB_off is confusing

thecppzoo · 2024-05-27T03:01:31Z

inc/zoo/swar/math.h

+std::enable_if_t<zoo::math::isPowerOfTwo<size_t, N>(), S>
+moduloPowerOfTwo(const S x) noexcept {
+    constexpr auto N_minus_1 = N - 1;
+    constexpr auto N_in_lanes = zoo::meta::BitmaskMaker<typename S::type, N_minus_1, S::NBits>::value;
+    auto y = x.value() & N_in_lanes;
+    return S{y};
+}
+


The way this is formulated is perhaps not useful:
For integral types, when the compiler can prove the modulus is a power of two, the compiler will, for sure, use bitwise conjunction rather than the assembler for the modulo instruction.
There remains the question of when the compiler doesn't know whether the modulus is a power of two, this would be only at runtime, and then, programmers have explicitly accounted for this improvement since ages, I don't think this way of going about it will give them an incentive to use this primitive.
This is useful for "weird" types that represent integers, like multi-precision integers, I don't know, that do not satisfy the "Integral" property required by isPowerOfTwo.
There is one aspect that I really like aobut this code: the structural guarantee that you won't pass a non-power-of-two as template argument, I see myself making some code with lane sizes not powers of two, and in some remote part of the code there is an assumption broken of the lane size being a powr of two, invoking bitwise conjuction, for "hilarity".

thecppzoo · 2024-05-27T03:01:58Z

test/swar/BasicOperations.cpp

@@ -1,11 +1,14 @@
+#include "zoo/swar/SWAR.h"


probably an auto-import

thecppzoo · 2024-05-27T08:37:56Z

test/swar/BasicOperations.cpp

+        const bool (&expected)[S::Lanes]) {
+    return  equals(S{S::Literal, left}, S{S::Literal, right}).value()


Accidental levels of indentation, scopes are not closed at the indentation level.
Do you think moving the end of the argument list to the right helps the code in any way?
Why is the declaration of the three arguments more nested than the implementation?

thecppzoo · 2024-05-27T08:42:58Z

test/swar/BasicOperations.cpp

-                     S{S::Literal,      {5, 2, 7, 4}}).value()
-                     == BS{BS::Literal, {F, T, F, T}}.value());
+template <typename S = S, typename BS = BS>
+constexpr auto laneWiseEqualsTest(


This is not right, if you want "all lanes equal" you use the underlying value comparison as in all other tests.
Unit tests ought to demonstrate how to use a codebase. Using per-lane comparisons for a whole-base-type comparison is inadequate.
There is a probem with equals which is that is terseness invites incorrect use.
Even if you use equals, the comparison against a BooleanSWAR of all true is inelegant:
not(boolean(~equals(, since we have the conversion to boolean

thecppzoo · 2024-05-27T08:44:01Z

test/swar/BasicOperations.cpp

+}
+static_assert(laneWiseEqualsTest({1, 2, 3, 4}, {1, 2, 3, 4}, {1, 1, 1, 1}));
+static_assert(laneWiseEqualsTest({1, 2, 3, 4}, {5, 6, 7, 8}, {0, 0, 0, 0}));
+static_assert(laneWiseEqualsTest({1, 2, 3, 4}, {5, 2, 7, 4}, {0, 1, 0, 1}));


This is legitimate as written

thecppzoo · 2024-05-27T08:44:47Z

test/swar/BasicOperations.cpp

+static_assert(laneWiseEqualsTest({1, 2, 3, 4}, {1, 2, 3, 4}, {1, 1, 1, 1}));
+static_assert(laneWiseEqualsTest({1, 2, 3, 4}, {5, 6, 7, 8}, {0, 0, 0, 0}));


These are whole-BooleanSWAR comparisons that can be achieved via the boolean conversion

thecppzoo · 2024-05-27T08:53:05Z

test/swar/BasicOperations.cpp

-                     S{S::Literal,      {5, 2, 7, 4}}).value()
-                     == BS{BS::Literal, {F, T, F, T}}.value());
+template <typename S = S, typename BS = BS>
+constexpr auto laneWiseEqualsTest(


laneWiseEquals is problematic:
What does it mean? there are results that are expected to be equal or different according to a collection of expected boolean values, that is not even suggested by the name.
Then, laneWiseEquals suggests LANE WISE EQUALS. I guess that you call your "expected" comparison "WISE EQUAL".
Notwithstanding other problems, perhaps lanewiseEqualityComparisonExpected?

thecppzoo · 2024-05-27T08:55:45Z

test/swar/BasicOperations.cpp

+}
+static_assert(powerOfTwoTest<S, BS>({1, 2, 3, 4}, {1, 1, 0, 1}));
+static_assert(powerOfTwoTest<S, BS>({2, 3, 64, 77}, {1, 0, 1, 0}));
+static_assert(powerOfTwoTest<S, BS>({3, 65, 128, 0}, {0, 0, 1, 1}));


This much copy&pasting is not valuable.
Did you consider packing all of the redundancy into an X macro?

thecppzoo

We need to document much better what are the primitives available and especially common idioms around them.
In this PR there is clear misuse of existing operations and the introduction of idioms that are not well justified, because the common idioms are not well known.
I've not worked in cleaning up the SWAR tests because I believe that they ought to be generated by a code generator where you express the intent and the test cases are generated and their source code.
We just have to stop making this more confusing by accidental misuse.

thecppzoo · 2024-05-27T09:03:06Z

The implementation look good.
I have a problem with the unit tests. Not even with what is tested, that seems right, but how things are tested, however semantically correct, they are misuse of the available operations.
A user attempting to use this operations with elegance and top performance would be more confused by these tests than without these tests. That users get confused with the codebase as it is, is powerful reason to not confuse more, not to accept more confusing tests because there are precedents of confusing tests.

jamierpond added 29 commits April 17, 2024 00:45

new literals

295d602

PR feedback

dbf8115

tidy tests

2327807

tidy reused code for booleanswaer

7c10d29

update name

0b8b214

style

e3a802d

test style

7ed6e88

fix

03edd02

undef util

0648bb1

simplify again

0d0a833

fmt

33183cd

Add to Array

a7d744d

add to_array oops make 23 for now not sure we want to commit to this? remove named function

rename

06af01c

nailed it

ffc1120

cleanup

5b837e6

array tests?

c97318f

indentaion

3dd0296

update indent

da9ccb9

from array

da64b7d

more updates

11ccd9e

wip

aab033a

rm unused

ba6a5ba

respect 80 chars

f3d9f42

undo clang formatting

783f189

format boolean swar

f9e28b1

improvementws

d636521

indentation

d880691

make sure we understand equality

bbad583

works

f6a04f1

jamierpond changed the base branch from master to jp/swar-to-array May 26, 2024 07:54

thecppzoo added 2 commits May 26, 2024 13:26

Update SWAR.h

bf93d56

Reverting mistake, this is not a `struct` definition but a variable!

Attempt to sidestep MSVC bug

79d3847

thecppzoo reviewed May 26, 2024

View reviewed changes

jamierpond added 5 commits May 26, 2024 17:10

camelCawe

a585ad9

Merge branch 'jp/swar-to-array' into jp/ml-basics

711bb08

indentatino

f1452cb

auto

f45632a

add utils

79148d3

thecppzoo reviewed May 27, 2024

View reviewed changes

jamierpond added 3 commits May 27, 2024 00:10

tidy up

08a73e1

weren't using that anyway

0228573

tidy up tests

0829a7e

thecppzoo reviewed May 27, 2024

View reviewed changes

thecppzoo requested changes May 27, 2024

View reviewed changes

jamierpond changed the base branch from jp/swar-to-array to master June 27, 2024 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement lane-wise modulo. #86

Implement lane-wise modulo. #86

jamierpond commented May 26, 2024 •

edited

Loading

thecppzoo May 26, 2024

Scottbruceheart Jun 3, 2024

thecppzoo May 26, 2024

jamierpond May 27, 2024

thecppzoo May 26, 2024

thecppzoo May 26, 2024

thecppzoo May 27, 2024

jamierpond May 27, 2024

thecppzoo commented May 27, 2024

thecppzoo May 27, 2024

jamierpond May 27, 2024

thecppzoo May 27, 2024 •

edited

Loading

jamierpond May 27, 2024

thecppzoo May 27, 2024

thecppzoo May 27, 2024

thecppzoo May 27, 2024

jamierpond May 27, 2024

thecppzoo May 27, 2024

thecppzoo May 27, 2024

thecppzoo May 27, 2024

thecppzoo May 27, 2024

thecppzoo May 27, 2024

thecppzoo May 27, 2024

thecppzoo left a comment •

edited

Loading

thecppzoo commented May 27, 2024 •

edited

Loading

		const bool (&expected)[S::Lanes]) {
		return equals(S{S::Literal, left}, S{S::Literal, right}).value()

		static_assert(laneWiseEqualsTest({1, 2, 3, 4}, {1, 2, 3, 4}, {1, 1, 1, 1}));
		static_assert(laneWiseEqualsTest({1, 2, 3, 4}, {5, 6, 7, 8}, {0, 0, 0, 0}));

Implement lane-wise modulo. #86

Are you sure you want to change the base?

Implement lane-wise modulo. #86

Conversation

jamierpond commented May 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thecppzoo commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thecppzoo May 27, 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

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

thecppzoo commented May 27, 2024 • edited Loading

jamierpond commented May 26, 2024 •

edited

Loading

thecppzoo May 27, 2024 •

edited

Loading

thecppzoo left a comment •

edited

Loading

thecppzoo commented May 27, 2024 •

edited

Loading