[SYCL][ESIMD] Add more stringent compile time checks for accessor versions of block_load/block_store, gather/scatter API

[fineg74]

No description provided.

[v-klochkov]

@fineg74 - thank you for the fix. It look very good in general!
I have only few comments. Also, this fix needs conflicts resolution.

[v-klochkov]

Looks good to me.
Because this patch touches several key memory functions, it would be good to have another look at it by @sarnex or @turinevgeny

[sarnex]

I think this allows local accessors now. We have

template <typename T, accessor_mode_cap_val_t Capability, typename RetT>
using EnableIfAccessor =
    std::enable_if_t<detail::is_accessor_with_v<T, Capability>, RetT>;

and is_accessor_with_v is

template <typename T, accessor_mode_cap_val_t Capability>
inline constexpr bool is_accessor_with_v =
    is_device_accessor_with_v<T, Capability> ||
    is_local_accessor_with_v<T, Capability>;

So won't local accessor be allowed because of the ||? Or did I misunderstand? I assume we don't want to allow local accessors here.

[fineg74]

Fixed

[sarnex]

Same Q as above

[v-klochkov]

Good catch.
This case was supposed to be supported in the series of PRs implementing passing local_accessor to accessor-based memory functions, but it is not.
We need variants of functions copy_from() & copy_to() accepting local accessor, but such variants need to accept uint32_t even with __ESIMD_FORCE_STATELESS_MEM.

This function here should not accept local accessor because it accepts uint64_t in stateless mode.

[v-klochkov]

copy_to() copy_from() need a separate overload accepting local accessor.

[fineg74]

Fixed. Introduced a separate version of copy_to/copy_from for local_accessors

[v-klochkov]

@fineg74 - thank you for the fix. It look very good in general! I have only few comments. Also, this fix needs conflicts resolution.

For the record: The new checks make the code much safer. Before this PR memory functions could write to memory referenced by read-only accessor, which could cause any problems such as not updated buffers after kernel run, etc.

The new checks do not yet verify the accessor element type. So, it is still possible write QWORDS to BYTE buffer ACCESSOR.
Such cases also may have some alignment issues. Perhaps, that can be fixed later in other PRs.

[fineg74]

@fineg74 - thank you for the fix. It look very good in general! I have only few comments. Also, this fix needs conflicts resolution.

For the record: The new checks makes the code much safer. Before this PR memory functions could write to memory referenced by read-only accessor, which could cause any problems such as not updated buffers after kernel run, etc.

The new checks do not yet verify the accessor element type. So, it is still possible write QWORDS to BYTE buffer ACCESSOR. Such cases also may have some alignment issues. Perhaps, that can be fixed later in other PRs.

This probably needs more discussion, as I am not sure if there are problems with different data types apart from alignment that would require enforcement of buffer type and accessor type being the same or probably the same size. Alignment is a separate story as technically BYTE buffer could be QWORD aligned by user which means we would need to introduce an additional parameter for the API to indicate buffer alignment.

[v-klochkov]

@fineg74 - thank you for the fix. It look very good in general! I have only few comments. Also, this fix needs conflicts resolution.

For the record: The new checks makes the code much safer. Before this PR memory functions could write to memory referenced by read-only accessor, which could cause any problems such as not updated buffers after kernel run, etc.
The new checks do not yet verify the accessor element type. So, it is still possible write QWORDS to BYTE buffer ACCESSOR. Such cases also may have some alignment issues. Perhaps, that can be fixed later in other PRs.

This probably needs more discussion, as I am not sure if there are problems with different data types apart from alignment that would require enforcement of buffer type and accessor type being the same or probably the same size. Alignment is a separate story as technically BYTE buffer could be QWORD aligned by user which means we would need to introduce an additional parameter for the API to indicate buffer alignment.

Yes, this definitely needs a special discussion. Accessor type-element checks are surely not for this PR.

[againull]

Failed task was restarted and passed.