Enable zip_iterator to satisfy C++20 std::input_iterator constraint

[mmichel11]

Currently with C++20, oneapi::dpl::zip_iterator cannot satisfy the C++20 std::input_iterator constraint due to it's failure to satisfy std::indirectly_readable which requires certain common references between a iterator's value and reference types.

To resolve this issue, we specialize std::basic_common_reference for oneapi::dpl::__internal::tuple to recursively fetch the common reference types of each corresponding tuple element in the input tuples.

Furthermore, we currently use std::tuple in the internal zip_forward_iterator. In our internal use case, this becomes problematic when oneapi::dpl::__internal::tuple is nested inside std::tuple and we attempt to find common reference types. This can be produced with transform_unary.pass and transform_binary.pass with the TBB backend, where we meet compile errors such as the following:

no type named 'type' in 'std::common_reference<std::tuple<oneapi::dpl::__internal::tuple<const int &>, oneapi::dpl::__internal::tuple<int &>> &&, std::tuple<oneapi::dpl::__internal::tuple<int>, oneapi::dpl::__internal::tuple<int>> &>'
 3616 |     using common_reference_t = typename common_reference<_Tp...>::type;

Tuple-like common reference types are not standardized until C++23 which seems to be the root of this error: https://en.cppreference.com/w/cpp/utility/tuple/basic_common_reference

To resolve such errors, I have switched zip_forward_iterator to use the internal tuple implementation in a manner consistent with zip_iterator. This will invoke the basic_common_reference specialization provided in this PR for oneapi::dpl::__internal::tuple and resolve the encountered compilation errors with oneTBB. Both zip_iterator and zip_forward_iterator will now satisfy the std::input_iterator concept.

[danhoeflinger]

This isn't an argument against these changes, but to provide some context / history:

In #1432 (still unmerged), we removed a similar change because zip_forward_iterator was only used on the host side, not in hetero / dpcpp backend, and therefore did not need the tuple to be trivially copyable.

[mmichel11]

For further context about the change to use oneapi::dpl::__internal::tuple in zip_forward_iterator here for reviewers:

I have tested with C++23 with gcc 13 and using std::tuple builds fine due to the spec's update to include tuple-like common references. However, for C++20 we have a gap since this is not provided in the standard. Using our own tuple resolves this gap as the standard allows us to define std::basic_common_reference for user-defined types with a very similar definition to what is provided in C++23.

Attempting to define std::basic_common_reference for std::tuple in this instance would be a spec violation and causes issues in practice, so I think just switching to our internal tuple is best.

[danhoeflinger]

I'm having trouble understanding TQual and UQual.

Are they just something which is required for this function for it to be of use to the Standard library implementation?
Perhaps it uses these to add type qualifiers like const to the types?

Its not very clear to me as it is written in the standard.

[mmichel11]

They are required for this function to be used by the standard library implementation. My understanding is that they add cvref-qualifiers to the types.

It is mentioned briefly in the explanation of std::common_reference's required behavior:

"Otherwise, if std::basic_common_reference<std::remove_cvref_t<T1>, std::remove_cvref_t<T2>, T1Q, T2Q>::type exists, where TiQ is a unary alias template such that TiQ is U with the addition of Ti's cv- and reference qualifiers, then the member type type names that type;"

[danhoeflinger]

One thing to note is that oneDPL's internal tuple may not support everything that std::tuple supports, we do have the ability to convert from one to the other, but that doesn't always allow full functionality. One such example is #1472.

I don't expect problems with this necessarily... however its worth acknowledging the potential risk of such a change.

I don't think I like the following option, but I'll mention it anyway for discussion:
We could have a tuple type which resolves to std::tuple in most cases, and only for the specific situation we need it (TBB backend & C++ 20 only?) would it resolve to oneDPL internal tuple. I think for C++23 and beyond, std::tuple should work just fine for these purposes because it will provide this specialization of std::basic_common_reference itself.

[mmichel11]

@rarutyun @danhoeflinger

I have looked a bit deeper to see if we can find a way to continue to use std::tuple in zip_forward_iterator with a basic_common_reference specialization. We run into the following compilation error in std::common_reference_with when using std::tuple:

/usr/include/c++/11/concepts:72:30: note: the expression ‘is_convertible_v<_From, _To> [with _From = std::tuple<double, double>&; _To = std::tuple<const double&, double&>]’ evaluated to ‘false’
   72 |     concept convertible_to = is_convertible_v<_From, _To>

I have looked into this error some more and it seems to be unavoidable with std::tuple in C++20 but is addressed with the changes to std::tuple in P2321R2 (zip) in C++23. The immediate compiler error we are hitting is that a tuple reference is not convertible to a tuple of references in C++20 but is in C++23. This is checked when finding a common reference between zip_forward_iterator's value_type& (tuple reference) and reference (tuple of references) in std::indirectly_readable.

The following godbolt example demonstrates the issue in C++20 and its resolution in C++23: https://godbolt.org/z/YzTje39K6.

With our internal tuple implementation, this is not an issue. My take is that we add some functionality to use std::tuple in zip_forward_iterator with C++17, switch to our internal tuple for C++20, and use std::tuple again with C++23 when we detect the appropriate feature macros. I am curious to hear your thoughts.

[danhoeflinger]

With our internal tuple implementation, this is not an issue. My take is that we add some functionality to use std::tuple in zip_forward_iterator with C++17, switch to our internal tuple for C++20, and use std::tuple again with C++23 when we detect the appropriate feature macros. I am curious to hear your thoughts.

I think this is a reasonable way to limit exposure of potential issues that could arise from switching to the internal tuple, and is probably the best path. It could be possible to further limit exposure by limiting it only to the backends for which this concept is used (TBB).

edit: However, without some known issue with our internal tuple to avoid, I don't think its necessary to limit only to the TBB backend, just raising it as an option.

[mmichel11]

With our internal tuple implementation, this is not an issue. My take is that we add some functionality to use std::tuple in zip_forward_iterator with C++17, switch to our internal tuple for C++20, and use std::tuple again with C++23 when we detect the appropriate feature macros. I am curious to hear your thoughts.

I think this is a reasonable way to limit exposure of potential issues that could arise from switching to the internal tuple, and is probably the best path. It could be possible to further limit exposure by limiting it only to the backends for which this concept is used (TBB).

edit: However, without some known issue with our internal tuple to avoid, I don't think its necessary to limit only to the TBB backend, just raising it as an option.

I have added these changes and used the exact library features macros to dictate whether to use std::tuple or oneapi::dpl::internal::__tuple. If we are using C++20 and both concepts and concepts library support is present, then we use oneapi::dpl::__internal::tuple. Once we see __cpp_lib_ranges_zip and __cpp_lib_ranges_zip, then we can switch back to std::tuple. With C++17, these feature macros will evaluate to 0 and std::tuple will be used.

I have chosen not to limit to just the TBB backend in this commit. My reasoning is that std::tuple seems to not work well with proxy iterators until C++23. Currently, we only see issues with the TBB backend but since this is a general issue with zip_forward_iterator and C++20 concepts, I have chosen to disable it with other backends as well. Let me know if you think more discussion is needed.

[danhoeflinger]

Looks close to me, minor comments to address.

[danhoeflinger]

LGTM, but I'd recommend circling back with @rarutyun as well.

[danhoeflinger]

I think the remaining concern is about changing the layout of zip_forward_iterator based upon the standard library version specified.

zip_forward_iterator is in the __internal namespace, but we had discussed possibly adding a comment to its definition which points out that this should stay "internal", because of the layout ABI breaking changes which occur with different standard library implementations.
It may not be clear to someone refactoring down the line that this would be problematic if they were wanting to expose it as a part of the public API.

I think such a comment is a good idea. Otherwise, I think this is the best we can do for this issue.

[mmichel11]

I think the remaining concern is about changing the layout of zip_forward_iterator based upon the standard library version specified.

zip_forward_iterator is in the __internal namespace, but we had discussed possibly adding a comment to its definition which points out that this should stay "internal", because of the layout ABI breaking changes which occur with different standard library implementations. It may not be clear to someone refactoring down the line that this would be problematic if they were wanting to expose it as a part of the public API.

I think such a comment is a good idea. Otherwise, I think this is the best we can do for this issue.

Thanks, I have added a comment to address this in the zip_forward_iterator class definition.

[danhoeflinger]

LGTM

[rarutyun]

After the discussion with @mmichel11 and going through the files it looks good to me except one thing