Move self_logand to the logical_and callable #1959

SadiinsoSnowfall · 2024-09-11T16:06:07Z

No description provided.

SadiinsoSnowfall · 2024-09-30T12:28:54Z

While attempting to fix this callable for the "no simd" configurations, I encountered the following problem:
Say we want to compile the following function:

auto test(logical<short> a, logical<wide<int>> b) {
    return logical_and(a, b);
}

Because of the rules defined by common_logical_t, this should return a logical<wide<int>>, as logical<wide> always takes priority. However, in "no simd" mode, this isn't the case and the logical_and callable attempts to return a logical<wide<short>>, failing to compile. This is because in this mode, the logical_and function is mapped over each element of the b wide value. In doing so, it is passed two logical_scalar_values of type logical<short> and logical<int> at every step. The problem being that in this case, the logical<short> type takes precedence.

The current fix involves using a convert operation before returning the result from the callable. This is a no-op in "simd" mode as the computed type and the returned type do match. A better fix to this problem would be to directly pass the computed type, in this case common_logical_t<T, U> to the map function called in the callable dispatch implementations.

jfalcou · 2024-09-30T14:41:59Z

A better fix to this problem would be to directly pass the computed type, in this case common_logical_t<T, U> to the map function called in the callable dispatch implementations.

This would also improve compile time by not having a convoluted map result computation.
@DenisYaroshevskiy maybe I should fix map outside of this PR then we can update this one and so we don't have the convert laying around.

SadiinsoSnowfall · 2024-10-16T10:01:39Z

waiting on the map2 branch

include/eve/module/core/regular/impl/simd/arm/sve/logical_and.hpp

SadiinsoSnowfall · 2024-11-29T10:00:14Z

After some talking we decided to change the behaviour of common_logical such that the returned type is always a logical of the type of the first of its parameter. This should be mirroring what is done for bitwise callables and fix the problem we were encountering with the callables using common_logical.

This is a breaking change.

jfalcou · 2024-12-05T13:31:53Z

@DenisYaroshevskiy looks like it works. Your opinion is welcome so we can move forward.

DenisYaroshevskiy

I don't like that there is a mess with conversions.
We do two or more conversions per operation.

I would like the logical callable to handle that.

I am ok if that happens with the "smaller logical result type"

include/eve/arch/cpu/logical_wide.hpp

include/eve/module/core/regular/impl/simd/arm/sve/logical_and.hpp

DenisYaroshevskiy · 2024-12-05T14:59:35Z

include/eve/module/core/regular/logical_and.hpp

    }
+  }
+}


Usually - this type of code would be in regular file (since you have the platform implementations.

Also - seems like conversions should be in behaviour, not here.

Unaddressed.

We do that a lot for small impl in a lot of places. Save on including a file for 5 lines.

We only do that if we don't have the special file includes. Either one file or everyone gets a file.

I mean - that's how it is now. Let's not make this harder.

OK this is true.
@SadiinsoSnowfall put it back in a common/ file

include/eve/module/core/regular/logical_and.hpp

SadiinsoSnowfall · 2024-12-10T16:09:33Z

Note: this PR now also changes the following callables: logical_notand, logical_andnot, logical_or, logical_notor, logical_ornot, logical_xor as they have a dependency on the return type of logical_and and had hardcoded return type in their generic impl.

Also:

changed test.hpp to add cartesian and cartesian_square, previously used in both logical_and and common_value unit tests.
hotfixed operator< in kumi, this fix from Joel will get into kumi mainline.
fixed friends functions to work with the new common_logical behaviour (they will be cleaned up in futures PRs).
changed the logical_xxx unit tests to perform some more cross-type tests and ensure that the new common_logical behaviour is used.
fixed hz_device to make it work with the new common_logical behaviour.

The new behaviour is the following: given two types A and B, both either bool or logical_value, the type returned from common_logical will be:

bool if both A and B are bools
A if B is a bool (or vice versa)
A if it is a simd_value and B isn't.
B if it is a simd_value and A isn't.
If both A and B are simd_values or scalar_values, the return type will be:
- The smallest of the two
- The "most unsigned" one
- The "most integral" one
- The first one (in the case both have the same size, signedness and integral-ness)

DenisYaroshevskiy

quick review

include/eve/arch/cpu/logical.hpp

include/eve/arch/cpu/logical_wide.hpp

include/eve/concept/value.hpp

include/eve/deps/kumi/tuple.hpp

include/eve/detail/function/simd/common/friends.hpp

test/test.hpp

DenisYaroshevskiy · 2024-12-16T12:29:04Z

test/unit/module/core/logical_ops.hpp

+}
+
+template<typename T, typename U>
+using priority_t = eve::as_logical_t<decltype(find_priority_type<T, U>())>;


I'm iffy about this:

It seems like you rewrote a chunk of common_logical a second time and compare them.

Is this a fare description?

I thought that comparing common_logical_t to itself would not be that useful. But without knowing exactly what types we are currently processing, it is difficult to know what should be the output of common_logical_t. If you want I can try to discriminate between floats, signed integrals and unsigned integrals where applicable to make the desired output more clear.

It seems like there is only two usages of priority_t.

Just be explicit about the things you test, don't try to write a function.

Writing a function to validate input is a good idea only if you can somehow write that function in a much easier way than the original.

those two usage are actually used in the test doign a cartesian product of tests over all combo of scalar.
This is not something we can verify easily without missing stuff.

test/unit/module/core/logical_ops.hpp

DenisYaroshevskiy · 2024-12-16T12:31:53Z

include/eve/traits/overload/default_behaviors.hpp

+        {
+          constexpr bool supports_map_no_conversion = requires{ map(this->derived(), x, xs...); };
+
+          if      constexpr (supports_map_no_conversion) return map(this->derived(), x, xs...);


I don't think we have a use case for this but - why "has_implementation" check is after supports_map_no_conversion?

Probably a question for @jfalcou, you don't need to fix this now.

this is a remnant from before. This is not requried anymore, @SadiinsoSnowfall reverted it.

DenisYaroshevskiy · 2024-12-16T12:35:01Z

include/eve/traits/overload/default_behaviors.hpp

+      if      constexpr (std::same_as<T, c_t>)                     return e;
+      else if constexpr (std::same_as<T, bool> || scalar_value<T>) return c_t{e};
+      else                                                         return detail::call_convert(e, as_element<c_t>{});
+    }


Why do you need this? I'd expect call_convert to handle them. The bool? Adding a bool in convert seems like the right fix here then.

Adding bool to convert and required tests is a bit big for this. Let's say this stay and later we PR convert for bool.

DenisYaroshevskiy · 2024-12-16T13:44:07Z

include/eve/module/core/regular/is_equal.hpp

@@ -106,15 +106,15 @@ namespace eve
  namespace detail
  {
    template<value T, value U, callable_options O>
-    EVE_FORCEINLINE constexpr common_logical_t<T,U>
+    EVE_FORCEINLINE constexpr auto


Why is the return type changing here? (not that I care, unless it changed).

This was a remnant from resolving the tuple problem. This should be fixed now.

DenisYaroshevskiy · 2024-12-16T13:45:19Z

include/eve/module/core/regular/logical_and.hpp

-    { return EVE_DISPATCH_CALL(a, b); }
+    template<typename T, typename U>
+    constexpr EVE_FORCEINLINE common_logical_t<T, U> operator()(T a, U b) const noexcept
+      requires (same_lanes_or_scalar<T, U> && !arithmetic_simd_value<T> && !arithmetic_simd_value<U>)


This is a very weird requirement. Can't you just require relaxed_logical_value on T and U?

See the comment in the file.

what comment.
Like - ok - this is an unacceptable requirement for an interface. We need something reasonable.

DenisYaroshevskiy · 2024-12-16T13:48:05Z

include/eve/module/core/regular/logical_and.hpp

-      if constexpr( scalar_value<T> && scalar_value<U> ) return r_t(a && b);
-      else return a && b;
+      if  constexpr (relaxed_logical_scalar_value<T>) return a && b;
+      else                              return bit_cast(a.bits() & b.bits(), as<T>{});


I don't understand this else. You are in cpu right?

Is this like what - logical_value && is_wide_logical? Can you be more explicit about that, this is very confusing.

At least a comment.

I added a comment to clarify.

DenisYaroshevskiy · 2024-12-16T13:48:46Z

include/eve/module/core/regular/logical_notand.hpp

-    { return EVE_DISPATCH_CALL(a, b); }
+    template<typename T, typename U>
+    constexpr EVE_FORCEINLINE common_logical_t<T, U> operator()(T a, U b) const noexcept
+      requires (same_lanes_or_scalar<T, U> && !arithmetic_simd_value<T> && !arithmetic_simd_value<U>)


Same comment here about the requirements.

I updated the answer to the review.

This is still not ok for an interface.

DenisYaroshevskiy · 2024-12-16T13:49:09Z

include/eve/module/core/regular/logical_and.hpp

    }
+  }
+}


Unaddressed.

DenisYaroshevskiy · 2024-12-19T14:37:29Z

include/eve/concept/scalar.hpp

+//! @{
+//! @concept relaxed_logical_scalar_value
+//! @brief The concept `relaxed_logical_scalar_value<T>` is satisfied if and only if T is a
+//! boolean value or satisfies the `eve::scalar_value` concept.


the scalar value here is ofc weird but w/e, I don't want to change it now.

DenisYaroshevskiy · 2024-12-19T14:38:15Z

include/eve/deps/kumi/tuple.hpp

      }
-      (std::make_index_sequence<sizeof...(Ts)-1>());
-      return res;
+      (std::make_index_sequence<sizeof...(Us)-1>());


What's with this kumi change? This should probably go in upstream first, no? Or do we need to upgrade in lockstep?

upstreal is fixed already as externals CI pass.
This is probably some noise and should be turned back to Ts.

include/eve/detail/hz_device.hpp

DenisYaroshevskiy · 2024-12-19T14:42:49Z

include/eve/module/core/regular/is_not_equal.hpp

  {
-    template<value T, value U>
+    template<typename T, typename U>
    requires(eve::same_lanes_or_scalar<T, U>)
    constexpr EVE_FORCEINLINE common_logical_t<T,U>  operator()(T a, U b) const
    {


the simd tolerance looks very weird, whatever that is.

DenisYaroshevskiy · 2024-12-19T14:43:52Z

include/eve/module/core/regular/is_not_equal.hpp

  {
-    template<value T, value U>
+    template<typename T, typename U>


is_not_equal is not a logical element wise callable.
It's just not.
We need to figure out what's up.

DenisYaroshevskiy · 2024-12-19T14:46:33Z

include/eve/module/core/regular/is_not_equal.hpp

        auto tmp  = is_not_equal(a, b);
-        if constexpr( floating_value<r_t> ) return tmp && (is_not_nan(a) || is_not_nan(b));
-        else                                return tmp;
+        if constexpr( floating_value<T> ) return tmp && (is_not_nan(a) || is_not_nan(b));


this is weird but it's not on you, i'll just ignore it.

DenisYaroshevskiy · 2024-12-19T14:47:53Z

include/eve/module/core/regular/logical_and.hpp

-    { return EVE_DISPATCH_CALL(a, b); }
+    template<typename T, typename U>
+    constexpr EVE_FORCEINLINE common_logical_t<T, U> operator()(T a, U b) const noexcept
+      requires (same_lanes_or_scalar<T, U> && !arithmetic_simd_value<T> && !arithmetic_simd_value<U>)


what comment.
Like - ok - this is an unacceptable requirement for an interface. We need something reasonable.

DenisYaroshevskiy · 2024-12-19T14:56:44Z

include/eve/module/core/regular/logical_and.hpp

    }
+  }
+}


We only do that if we don't have the special file includes. Either one file or everyone gets a file.

I mean - that's how it is now. Let's not make this harder.

DenisYaroshevskiy · 2024-12-19T14:57:19Z

include/eve/module/core/regular/logical_notand.hpp

-    { return EVE_DISPATCH_CALL(a, b); }
+    template<typename T, typename U>
+    constexpr EVE_FORCEINLINE common_logical_t<T, U> operator()(T a, U b) const noexcept
+      requires (same_lanes_or_scalar<T, U> && !arithmetic_simd_value<T> && !arithmetic_simd_value<U>)


This is still not ok for an interface.

SadiinsoSnowfall marked this pull request as draft September 11, 2024 17:27

SadiinsoSnowfall force-pushed the callable-logand branch 4 times, most recently from af4d016 to b0012c1 Compare September 19, 2024 08:28

SadiinsoSnowfall force-pushed the callable-logand branch from da4a721 to e10f67e Compare September 26, 2024 14:03

SadiinsoSnowfall marked this pull request as ready for review September 26, 2024 14:56

SadiinsoSnowfall force-pushed the callable-logand branch from 48d1d7b to 9e3c740 Compare September 30, 2024 09:38

SadiinsoSnowfall marked this pull request as draft October 16, 2024 10:01

SadiinsoSnowfall added 15 commits November 29, 2024 10:44

take 1

22ff6e8

added backends

c165f5a

typo

011a5c3

allow booleans in common_logical

54b73bd

fixed common_logical impl

40c67a6

fix

f2a1dc8

fixed missing case for bool x bool

131b561

riscv fix

29abcbd

avx512 fix

7ea3c77

fix resolution problem

07fd843

fix

b309af1

made common_logical more commutative

3594603

fix NO_SIMD

959832b

fix

374bbc0

changed common_logical behaviour

c7bdc8a

DenisYaroshevskiy reviewed Nov 29, 2024

View reviewed changes

include/eve/module/core/regular/impl/simd/arm/sve/logical_and.hpp Outdated Show resolved Hide resolved

SadiinsoSnowfall force-pushed the callable-logand branch from fe64b25 to c7bdc8a Compare November 29, 2024 11:06

DenisYaroshevskiy reviewed Dec 5, 2024

View reviewed changes

SadiinsoSnowfall added 3 commits December 6, 2024 15:55

new behaviour

43e77ee

[ci skip] stash

cb68b56

fix

98a2a82

SadiinsoSnowfall added 4 commits December 11, 2024 09:55

fixed NOSIMD + improved operator|| codegen + fixed shadowing

413396c

fix

d9c231e

fix typo

75ec64f

fix impls

c9b000c

SadiinsoSnowfall force-pushed the callable-logand branch from 4f076e5 to c9b000c Compare December 12, 2024 10:01

SVE fix

d88cb08

DenisYaroshevskiy reviewed Dec 12, 2024

View reviewed changes

SadiinsoSnowfall added 2 commits December 13, 2024 09:14

review fixes

0e35da3

rename concept

eba6e03

DenisYaroshevskiy reviewed Dec 16, 2024

View reviewed changes

SadiinsoSnowfall and others added 10 commits December 16, 2024 22:00

fix missing includes in wide

8f33961

moved relaxed_logical_scalar_value

eaaa7d0

renamed logical_ops to logical_test

25eb612

revert useless changes to strict_elementwise_callable

1882295

adjusted comment

9903a9b

refactor AVX512 code

1cae9a1

revert is_equal return types

b446032

more comments

742d79d

preemptive fix for comparison operators

6a33114

Clarify logical_elementwise_callable behavior

1d51da6

DenisYaroshevskiy reviewed Dec 19, 2024

View reviewed changes

jfalcou mentioned this pull request Dec 19, 2024

Provide cartesian_product of types<...> jfalcou/tts#104

Open

SadiinsoSnowfall added 2 commits December 20, 2024 20:15

review fix

a87e245

remove logical_elementwise_callable

753bbeb

Move self_logand to the logical_and callable #1959

Are you sure you want to change the base?

Move self_logand to the logical_and callable #1959

Conversation

SadiinsoSnowfall commented Sep 11, 2024

SadiinsoSnowfall commented Sep 30, 2024

jfalcou commented Sep 30, 2024

SadiinsoSnowfall commented Oct 16, 2024

SadiinsoSnowfall commented Nov 29, 2024

jfalcou commented Dec 5, 2024

DenisYaroshevskiy 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

SadiinsoSnowfall commented Dec 10, 2024

DenisYaroshevskiy 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

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

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