Enable binary comparison operations for internal tuple

[danhoeflinger]

Summary

This PR enables comparison <, >,<=,>= of our internal tuple oneapi::dpl::__internal::tuple<T...> with another oneapi::dpl::__internal::tuple<U...> or std::tuple<U...> with equal number of elements where corresponding elements from rhs and lhs are comparable.

We use the std::tuple implementation for these operations by static casting and repeating the operation.

We extend the same to == and !=, where we previously implemented the operation directly.

Details

We already provide an implicit conversion operator to a std::tuple, and also are able to construct from a std::tuple. However, when a template function is considered for overload resolution it does type deduction and will only use exact matches. It will not use any implicit conversion to build the candidate list. Therefore, this implicit conversion is not used when searching for resolution for operator<, and we cannot take advantage of std::tuple's implementation of these operators (at least without extra effort).

We use a macro to unify implementation of all binary operators in the same fashion and reduce code to a minimum.

[tomflinda]

LGTM

[adamfidel]

Slightly pedantic, but std:: should be ::std:: to match the rest of the methods in this class.

[danhoeflinger]

So, I think our mindset has changed on ::std vs std. Tagging @MikeDvorskiy. We had talked about adding this to some sort of a style document or at least making an announcement, but that hasn't happened yet.

I was told that it is unnecessary to use ::std vs std and that new code should be the later. Similar to formatting though, we don't want to ruin the "blame" by changing it aggressively everywhere.

[adamfidel]

Ah, okay. I wasn't aware of this discussion. Better to stick with std:: here in that case.

[danhoeflinger]

Yes, we need to propagate that decision out and also codify it somewhere here in the repo in some sort of a style guide, I think. We could even simply add a section to CONTRIBUTING.md with some general rules and best practices.

[danhoeflinger]

I'm working on adding some unit testing for tuple to the test suite. Once I add that, I will probably revert the changes to zip_iterator.pass .

[adamfidel]

I was curious if the three provided functions in the macro were enough to cover all of the cases to compare. But I enumerated them and it does seem like it is sufficient.

There are three tuple types we would like to compare:

1. tuple (self with T1, T...)
2. tuple<_U1, _U...> (self with potentially different types)
3. std::tuple<_U1, _U...> (standard tuple with potentially different types

lhs	rhs	matching candidate
1	1	__OPERATOR(const tuple& __lhs, const oneapi::dpl::__internal::tuple<_U1, _U...>& __rhs)
1	2	__OPERATOR(const tuple& __lhs, const oneapi::dpl::__internal::tuple<_U1, _U...>& __rhs)
1	3	__OPERATOR(const tuple& __lhs, const std::tuple<_U1, _U...>& __rhs)
2	1	__OPERATOR(const tuple& __lhs, const oneapi::dpl::__internal::tuple<_U1, _U...>& __rhs)
2	2	__OPERATOR(const tuple& __lhs, const oneapi::dpl::__internal::tuple<_U1, _U...>& __rhs)
2	3	__OPERATOR(const tuple& __lhs, const std::tuple<_U1, _U...>& __rhs)
3	1	__OPERATOR(const std::tuple<_U1, _U...>& __lhs, const tuple& __rhs)
3	2	__OPERATOR(const std::tuple<_U1, _U...>& __lhs, const tuple& __rhs)
3	3	defined by standard

Based on this, it seems like the three functions that are defined are sufficient.

[danhoeflinger]

Based on this, it seems like the three functions that are defined are sufficient.

Yes, thanks for doing this enumeration of the possibilities. Working on a test which should prove that out as well.

[danhoeflinger]

OK, now all those options @adamfidel mentioned should be covered by the unit test. I've reverted the zip_iterator test changes in favor of unit testing.

I've also rebased to have 2 commits so that we can separate the unit test addition in the main git log from the tuple changes.

[SergeyKopienko]

I think better if we will support all restrictions for these operators like in https://en.cppreference.com/w/cpp/utility/tuple : all these operators will be removed in C++20.

[SergeyKopienko]

Does it make sense to add tests for Kernels too?

[danhoeflinger]

Do you mean calling sycl kernels or oneDPL algos with oneDPL internal tuples? Checking is_trivially_copyable should check that this is possible. Also, we have other tests which utilize zip_iterator with the algorithms which should provide system level testing.

My goal when adding this new test is to unit test tuple for some specific attributes we want it to have.

[danhoeflinger]

I think better if we will support all restrictions for these operators like in https://en.cppreference.com/w/cpp/utility/tuple : all these operators will be removed in C++20.

@SergeyKopienko
Can you be more specific about what you mean by this comment? Do you mean we should provide a different implementation for C++20 guarded by preprocessor logic?

Also, these operators are removed in C++20, but only in favor of the operator<=> which supports all of them with a single operator. From the outside looking in, there shouldn't be a difference.

[SergeyKopienko]

I think better if we will support all restrictions for these operators like in https://en.cppreference.com/w/cpp/utility/tuple : all these operators will be removed in C++20.

@SergeyKopienko Can you be more specific about what you mean by this comment? Do you mean we should provide a different implementation for C++20 guarded by preprocessor logic?

Also, these operators are removed in C++20, but only in favor of the operator<=> which supports all of them with a single operator. From the outside looking in, there shouldn't be a difference.

For me it's looks like we should implement all these operators only for C++17 version and implement <=> operator for C++20.

[danhoeflinger]

For me it's looks like we should implement all these operators only for C++17 version and implement <=> operator for C++20.

That's fine. I can do that.

[danhoeflinger]

For me it's looks like we should implement all these operators only for C++17 version and implement <=> operator for C++20.

I'm wondering if it is a good idea after some exploration...

The way it is currently, it works for both C++17 and C++20 and offers the support and comparison with std::tuple.

If we change it to be operator<=> when C++20 is defined (removing the individual operators), we need to be in lock step with when std::tuple::operator<=> was included in the implementation of C++20 features, otherwise it will be broken. If we include both individual operators and <=> at the same time, it is technically ambiguous and will at least provide warnings.

I think the only thing we miss out on in c++20 with the current implementation is if someone explicitly uses the <=> operator (which seems both unlikely and not a case we care much about).

[SergeyKopienko]

For me it's looks like we should implement all these operators only for C++17 version and implement <=> operator for C++20.

I'm wondering if it is a good idea after some exploration...

The way it is currently, it works for both C++17 and C++20 and offers the support and comparison with std::tuple.

If we change it to be operator<=> when C++20 is defined (removing the individual operators), we need to be in lock step with when std::tuple::operator<=> was included in the implementation of C++20 features, otherwise it will be broken. If we include both individual operators and <=> at the same time, it is technically ambiguous and will at least provide warnings.

I think the only thing we miss out on in c++20 with the current implementation is if someone explicitly uses the <=> operator (which seems both unlikely and not a case we care much about).

Understood, ok.

[MikeDvorskiy]

Let me ask: what's a motivation for that internal::tuple functionality extending?
Other words, where in OneDPL impl do we need such functionality?

[danhoeflinger]

Let me ask: what's a motivation for that internal::tuple functionality extending? Other words, where in OneDPL impl do we need such functionality?

Example:
Someone has a zip_iterator of two sequences of integers zipped together. They call oneapi::dpl::sort with no comparator. This will compare each element (onedpl tuple) using operator<. There is a well defined semantic for comparing std::tuple with operator<, so it seems a case we should handle (There is such a use case coming from SYCLomatic).

Then, if we are covering this case, why should it work with one such operator and not others?

Similarly, since we return this tuple type from zip_iterator where the user may then use it, why not allow it to have the same semantics (comparable with different element types) and be interoperable with std::tuple?

I'd say the most important use case is to (1) allow binary comparison between two same element-typed internal tuples as that is what we may encounter within oneDPL algorithm calls. This allows a much simpler implementation.

Second most interesting would be (2) comparisons between onedpl tuples with different element types as users could contrive examples which this is necessary inside of a oneDPL call.

Least interesting is (3) interoperability with std::tuple (it is a "nice to have" feature in my view).

I'd be willing to drop (3) from this PR, it causes the most complication in implementation and has the least utility.

[danhoeflinger]

I'd be willing to drop (3) from this PR, it causes the most complication in implementation and has the least utility.

I've re-organized the code to make clear which code is to support which functionality. The first set of operators covers 1+2, the second and third sets of operators are required to enable (3).

(If we wanted to only support (1), we could reduce the complexity of the first set of operators to just use const tuple& type for rhs and lhs)

[danhoeflinger]

After some help from @akukanov, I was able to consolidate 3 the different implementations of the comparators using some trait definitions.

Its a little tricky because we have to avoid ambiguous definitions of the same comparator from two separate instantiations of the internal tuple. The logic in the current set of enable_if is able to work by only defining comparisons between two different internal tuples in the instantiation matching the lhs type.

[akukanov]

The implementation looks good to me. The only question is whether oneapi::dpl::__internal namespace "prefix" is justified when used within the same namespace; but I do not mind it either way.
I have not reviewed the test.