Add more forward declarations to sycl_traits.h

[adamfidel]

The kernel template tests no longer compile after #1394 due to missing forward declarations. This PR gets the tests to compile again.

[dmitriy-sobolev]

Should we forward declare everything what is used in that file?

For example, there are entities, which miss their declarations:

• predicates/functors defined in a block starting with oneapi::dpl::__internal::__not_pred
• zip_iterator, transform_iterator, permutation_iterator

[adamfidel]

Should we forward declare everything what is used in that file?

That's a good question. I can certainly add forward declarations to all of the structs in this file, but I am not clear on the original design intent as it was introduced in #1394. I believe it was a design decision to only include some of the forward declarations in this file with the intent that the other structs will already be defined before including this header.

With that in mind, it might be better for the original author @MikeDvorskiy to answer that question.

[MikeDvorskiy]

Why the PR is still draft?
I believe #include <oneapi/dpl/internal/binary_search_extension_defs.h> is not quite correct, should be a relative patch via ""
I think the forward declarations are required here just for the types which are not already defined.

[dmitriy-sobolev]

I think the forward declarations are required here just for the types which are not already defined.

@MikeDvorskiy, do you confirm that it is everything besides what is declared in "binary_search_extension_defs.h" (a single include file here)? I just what to get an explicit answer to #1497 (review) regarding addition of the forward declarations listed there.

[danhoeflinger]

I really don't like the maintenance problem that #1394 presents. We discussed it back when #1394 was proposed and decided that it was the lesser of two evils to keep all these device_copyable definitions in one place rather than infect the rest of the code with device_copyable (SYCL specific) stuff.

However, the problems this has presented, and the need for all the forward declarations is making me reconsider that.

Would we be better off to just define the device copyable macro stuff in one place like this header or some other central header, and embed the _ONEDPL_DEVICE_COPYABLE( ... ); call in with the definition of the struct?

For non-dpcpp backends, the macro could be a no-op. It would reduce our maintenance burden by not having these duplicate declarations which need to be updated anytime something is changed. It will also make it more likely that when someone adds a similar functor or type to one which requires device copyable that they realize that they may need to add the new functor or type as _ONEDPL_DEVICE_COPYABLE() as well.

[adamfidel]

Why the PR is still draft? I believe #include <oneapi/dpl/internal/binary_search_extension_defs.h> is not quite correct, should be a relative patch via "" I think the forward declarations are required here just for the types which are not already defined.

This PR was waiting on #1496 to be merged. It is no longer a draft now. I also removed the changes to binary search so there are no longer any additional includes.

[danhoeflinger]

@adamfidel I think you need to rebase / resolve conflicts to fix the diff for this PR after merging #1496.

[danhoeflinger]

@adamfidel:

@MikeDvorskiy and I discussed a couple other issues we found with device copyable, and he is working on a PR which would likely supersede this PR by embedding the "device copyable" specializations with the types themselves, rather than having them in this sycl_traits.h.

I've created this issue #1505 to represent that, until a PR exists.

[akukanov]

After all investigations and prototypes made, it seems the current approach is still preferable, despite its shortcomings. Yes it's bad at maintenance, but another possible solution is not much better in this aspect and quite worse in others, and a solution which would really be better either needs to be supported by the implementation of sycl::is_device_copyable or requires C++20 concepts.

Therefore I think for now it's best to proceed with this patch.

[danhoeflinger]

After all investigations and prototypes made, it seems the current approach is still preferable, despite its shortcomings. Yes it's bad at maintenance, but another possible solution is not much better in this aspect and quite worse in others, and a solution which would really be better either needs to be supported by the implementation of sycl::is_device_copyable or requires C++20 concepts.

Therefore I think for now it's best to proceed with this patch.

I need to catch up on the discussion of the other approach(es), but the version we were working on also addresses 2 additional issues which were not covered by this approach. If we continue with this approach, we still need more than this PR to address the issues. Where the specializations live is a related but separate decision. I'd hold on merging this until we have a plan for all the issues we currently face.

1. Non-type template parameters (they do not match with a generic variadic template definition)
2. Subset of template parameters (specifically avoiding execution policies, which are passed as template params and include a non-device copyable queue but go unused and don't change device copyability of the type)

[akukanov]

Where the specializations live is a related but separate decision. I'd hold on merging this until we have a plan for all the issues we currently face.

That's simple - remove the macro, declare each specialization of sycl::is_device_copyable separately without any attempt to generalize. Or, if you prefer, keep using the macro where it works, otherwise do as suggested above.

[danhoeflinger]

@adamfidel I've created a PR targeting this PR #1520 to add some more missing fwd declarations. With those I think the list is complete. Please merge into this PR if you agree with that patch.

As mentioned in that PR, I think we still need to convert to the original proposal from #1513 (This commit), to allow a subset of types to be specified, and convert to that method at least for types which require a subset.

I've double checked the existing fwd declarations from this PR that they match the definition.

[danhoeflinger]

LGTM but please get a second approval since I added code here. (and wait for green CI)
I spoke with @MikeDvorskiy and we decided its best to merge this first then address the rest in a separate PR.

[MikeDvorskiy]

LGTM