[SYCL] Change trivially_copyable to device_copyable

[hdelan]

cgh.fill(...) was failing when T is a sycl::vec type, since sycl::vec types have no specialization for std::is_trivially_copyable. Since device_copyable is a sycl superset of trivially copyable we should use this instead in sycl headers.

I can add a test to check that cgh.fill works for vec types if necessary. Let me know if I should do this.

There are some other occurrences of std::is_trivially_copyable in the sycl headers. Let me know if I should replace more incidences.

[aelovikov-intel]

Looks reasonable to me. Tagging @gmlueck and @AlexeySachkov just in case..

[AlexeySachkov]

Looks reasonable to me. Tagging @gmlueck and @AlexeySachkov just in case..

The change seem to be ok from the point of view of our implementation, where we use custom kernel to implement fill. However, the spec currently does not impose any restrictions on T, so strictly speaking, both our implementations (existing one and proposed here) are incorrect, because they impose restrictions on T.

I wouldn't say that the spec is clear here as well, it says:

Replicates the value of src

Is src being copy-constructed to fill a buffer? Is it being simply byte-copied to fill a buffer? In OpenCL, corresponding interface does plain memcpy, but it accepts a pointer and size pair, it does not accept generic type T, which may not be trivially copyable, copy-constructible, or copy-assignable.

Since SYCL interface is a C++ interface accepting a reference to generic type T, I think that we should clarify in the spec which operations on T must be available for the implementation. Would be good to hear from @gmlueck here.

I'm still fine with merging this PR, though: it doesn't make the situation worse and actually relaxes restrictions we impose on T a bit.

[gmlueck]

Since SYCL interface is a C++ interface accepting a reference to generic type T, I think that we should clarify in the spec which operations on T must be available for the implementation. Would be good to hear from @gmlueck here.

I agree that this is unclear in the spec. I was looking at the git history of the spec, and I see that we used to say that T had to be trivially copyable, which explains the state of our implementation (before this PR). This wording got removed from the spec before it was published.

I think it is correct to restrict T to "device copyable", which means we can do a byte copy when implementing fill, so I agree with this PR.

I think we should make the same clarification for this member function of handler:

template <typename T> void copy(const T* src, T* dest, size_t count)

Do you have any objections if I clarify the spec for both fill and copy to say that T must be device copyable?

[hdelan]

Sorry I thought that I had responded to this.

Do you have any objections if I clarify the spec for both fill and copy to say that T must be device copyable?

Yes @gmlueck I'd be happy if you clarified this in the spec. I have also added some static_asserts for copy(const T *src) as well as the smart pointer case to make sure that T is device_copyable. Hopefully this can be merged

[gmlueck]

Sorry I thought that I had responded to this.

Do you have any objections if I clarify the spec for both fill and copy to say that T must be device copyable?

Yes @gmlueck I'd be happy if you clarified this in the spec. I have also added some static_asserts for copy(const T *src) as well as the smart pointer case to make sure that T is device_copyable. Hopefully this can be merged

I submitted a PR to the spec: KhronosGroup/SYCL-Docs#425

[dm-vodopyanov]

Failure in post-commit for E2E tests on Windows:

Failed Tests (1):
  SYCL :: Basic/handler/handler_mem_op.cpp

https://github.com/intel/llvm/actions/runs/5244433715/jobs/9471540544

@hdelan could you please address?

Upd: post commit on Linux is also failing: https://github.com/intel/llvm/actions/runs/5245429333/jobs/9473293760

[0x12CC]

#9829 fixes the E2E test failure caused by this PR.

[hdelan]

Sorry I was on holiday the last few days. Is this resolved by #9829 ?