llvm

llvm copied to clipboard

This PR prevents the buffer_allocator from being rebound to a const type. Currently when a buffer is reinterpreted from type T to type const T the buffer_allocator type is also changed to const T. This does not agree with the SYCL spec which states "A buffer of data type const T uses buffer_allocator<T> by default."

This PR also adds a compile time test which validates the returned type.

@AidanBeltonS Hm, to me the spec is not very clear here:

  template <typename ReinterpretT, int ReinterpretDim = Dimensions>
  buffer<ReinterpretT, ReinterpretDim,
         typename std::allocator_traits<AllocatorT>::template rebind_alloc<
             ReinterpretT>>
  reinterpret() const;

It suggests creating an allocator with the type which a buffer is being reinterpreted to - ReinterpretT, while in the buffer template parameters it explicitly states that the const should be removed:

template <typename T, int Dimensions = 1,
          typename AllocatorT = buffer_allocator<std::remove_const_t<T>>>
class buffer {

@gmlueck Could you please comment here?

If the const should be removed during for the reinterpreted buffer I would suggest adding std::remove_const_t<T>> in the reinterpret method template parameters, like this:

  template <typename ReinterpretT, int ReinterpretDim = Dimensions>
  buffer<ReinterpretT, ReinterpretDim,
         typename std::allocator_traits<AllocatorT>::template rebind_alloc<
             std::remove_const_t<ReinterpretT>>>
  reinterpret() const;

Hm, to me the spec is not very clear here:

I agree that the spec is not clear. I also agree that your proposed solution is better than changing how the rebinding occurs. I have proposed a fix to the SYCL Spec to update this interface as I expect this is a mistake. https://github.com/KhronosGroup/SYCL-Docs/pull/286

@romanovvlad the change to the Khronos spec has been merged, clarifying this issue. I have updated the PR to change the interface rather than the rebind operation. It seems like the ESIMD CI is failing, I am not sure if this is related to my changes. Is the current CI failure a known issue?

@romanovvlad the change to the Khronos spec has been merged, clarifying this issue. I have updated the PR to change the interface rather than the rebind operation. It seems like the ESIMD CI is failing, I am not sure if this is related to my changes. Is the current CI failure a known issue?

I guess CI fails are known issues and they should disappear after https://github.com/intel/llvm-test-suite/commit/7f0e3f0876f483790f47e29e5441683e56cb5497.

@romanovvlad has the CI been fixed yet? If so could we rerun the tests? Many thanks

@romanovvlad has the CI been fixed yet? If so could we rerun the tests? Many thanks Restarted the jobs.

@romanovvlad, @steffenlarsen could this be merged at some point, or is this waiting on more reviews?

Looks like the spec changes have been merged too, so I believe it is ready. Merging.