[SYCL][ESIMD] Add more stringent compile time checks to atomic_update API #11683

fineg74 · 2023-10-27T00:08:43Z

No description provided.

# Conflicts: # sycl/include/sycl/ext/intel/esimd/memory.hpp

sycl/include/sycl/ext/intel/esimd/memory.hpp

Fix several issues with the existing API

sycl/include/sycl/ext/intel/esimd/memory.hpp

…orCheck3' of https://github.com/fineg74/llvm into accessorCheck3

# Conflicts: # sycl/include/sycl/ext/intel/esimd/memory.hpp

v-klochkov

Please see some comment below.

sycl/include/sycl/ext/intel/esimd/memory.hpp

sycl/test/esimd/flat_atomic.cpp

# Conflicts: # sycl/include/sycl/ext/intel/esimd/memory.hpp

Co-authored-by: Vyacheslav Klochkov <[email protected]>

sycl/include/sycl/ext/intel/esimd/memory.hpp

… accessorCheck3 # Conflicts: # sycl/include/sycl/ext/intel/esimd/memory.hpp

sarnex · 2023-12-13T20:07:36Z

sycl/include/sycl/ext/intel/esimd/memory.hpp

@@ -5716,10 +5707,9 @@ template <atomic_op Op, typename T, int N, typename Toffset,
          typename AccessorTy,
          typename PropertyListT =
              ext::oneapi::experimental::detail::empty_properties_t>
-__ESIMD_API __ESIMD_API std::enable_if_t<
-    __ESIMD_DNS::get_num_args<Op>() == 0 && std::is_integral_v<Toffset> &&


Is removing the is_integral_v check international? I see it in a few places and just want to make sure.

I think, this change here is good. It is better to let this function be matched and be lowered to (acc-au0-1) where the check already exists:

llvm/sycl/include/sycl/ext/intel/esimd/memory.hpp

Line 5731 in 6a74b23

static_assert(std::is_integral_v<Toffset>, "Unsupported offset type");

I mean static_assert with a meaningful user-friendly error is better, than error "could not match" with a long list of prototypes that could not be matched.

It's removed in many places, so lets at least make sure every call tree has the static assert somewhere

That was the purpose of removing it as it doesn't make sense to have it both as static_assert and as a condition in enable_if_t

sarnex · 2023-12-13T20:10:27Z

sycl/include/sycl/ext/intel/esimd/memory.hpp

-          typename RegionTy = region1d_t<T, N, 1>,
-          typename PropertyListT =
-              ext::oneapi::experimental::detail::empty_properties_t>
+template <atomic_op Op, typename Tx, int N, typename Toffset,


Can we use T instead of Tx? We fixed that in most places when updating to the new APIs but might have missed it here.

sycl/include/sycl/ext/intel/esimd/memory.hpp

v-klochkov

Looks good to me. I have only 1 nit comment for intendations in comment-sections (the noticed pattern is met several times in the code).

sycl/include/sycl/ext/intel/esimd/memory.hpp

Need to fix "error: use of undeclared identifier 'Tx" error.

fineg74 added 3 commits October 26, 2023 13:11

Introduce more stringent checks for atomic_update API

a7f4098

Merge remote-tracking branch 'origin/sycl' into accessorCheck3

c9bfece

# Conflicts: # sycl/include/sycl/ext/intel/esimd/memory.hpp

Add additional tests for block_load/store API

f2b0c38

fineg74 requested a review from a team as a code owner October 27, 2023 00:08

fineg74 temporarily deployed to WindowsCILock October 27, 2023 00:09 — with GitHub Actions Inactive

fineg74 temporarily deployed to WindowsCILock October 27, 2023 00:31 — with GitHub Actions Inactive

Merge remote-tracking branch 'origin/sycl' into accessorCheck3

cc1ce4c

# Conflicts: # sycl/include/sycl/ext/intel/esimd/memory.hpp

fineg74 had a problem deploying to WindowsCILock December 1, 2023 23:59 — with GitHub Actions Failure

fineg74 temporarily deployed to WindowsCILock December 2, 2023 00:20 — with GitHub Actions Inactive

v-klochkov reviewed Dec 2, 2023

View reviewed changes

sycl/include/sycl/ext/intel/esimd/memory.hpp Outdated Show resolved Hide resolved

fineg74 added 2 commits December 3, 2023 22:05

Update the new API with checks,

b4ca8fe

Fix several issues with the existing API

Fix test failures

9671245

fineg74 temporarily deployed to WindowsCILock December 4, 2023 20:45 — with GitHub Actions Inactive

fineg74 temporarily deployed to WindowsCILock December 4, 2023 21:06 — with GitHub Actions Inactive

v-klochkov requested a review from turinevgeny December 4, 2023 23:35

v-klochkov reviewed Dec 5, 2023

View reviewed changes

sycl/include/sycl/ext/intel/esimd/memory.hpp Outdated Show resolved Hide resolved

Address PR comments

885e963

fineg74 temporarily deployed to WindowsCILock December 5, 2023 21:10 — with GitHub Actions Inactive

fineg74 temporarily deployed to WindowsCILock December 5, 2023 21:30 — with GitHub Actions Inactive

fineg74 added 3 commits December 5, 2023 22:33

Merge branches 'sycl', 'accessorCheck3', 'accessorCheck3' and 'access…

c3b734a

…orCheck3' of https://github.com/fineg74/llvm into accessorCheck3

Merge remote-tracking branch 'origin/sycl' into accessorCheck3

098ac3e

Simplify the compile time checks

b10017b

fineg74 temporarily deployed to WindowsCILock December 7, 2023 02:48 — with GitHub Actions Inactive

fineg74 temporarily deployed to WindowsCILock December 7, 2023 03:08 — with GitHub Actions Inactive

fineg74 mentioned this pull request Dec 7, 2023

[NFC][ESIMD] Fix incorrect declarations of simd_view based API #12093

Merged

Merge remote-tracking branch 'origin/sycl' into accessorCheck3

339e376

# Conflicts: # sycl/include/sycl/ext/intel/esimd/memory.hpp

v-klochkov reviewed Dec 7, 2023

View reviewed changes

Merge remote-tracking branch 'origin/sycl' into accessorCheck3

fea8a3d

# Conflicts: # sycl/include/sycl/ext/intel/esimd/memory.hpp

fineg74 temporarily deployed to WindowsCILock December 8, 2023 01:46 — with GitHub Actions Inactive

fineg74 temporarily deployed to WindowsCILock December 8, 2023 02:10 — with GitHub Actions Inactive

fineg74 and others added 3 commits December 8, 2023 09:38

Update sycl/include/sycl/ext/intel/esimd/memory.hpp

134f11c

Co-authored-by: Vyacheslav Klochkov <[email protected]>

Update sycl/include/sycl/ext/intel/esimd/memory.hpp

b55f7d4

Co-authored-by: Vyacheslav Klochkov <[email protected]>

Update sycl/test/esimd/flat_atomic.cpp

02ce465

Co-authored-by: Vyacheslav Klochkov <[email protected]>

fineg74 temporarily deployed to WindowsCILock December 8, 2023 17:59 — with GitHub Actions Inactive

fineg74 temporarily deployed to WindowsCILock December 8, 2023 18:32 — with GitHub Actions Inactive

v-klochkov reviewed Dec 8, 2023

View reviewed changes

sycl/include/sycl/ext/intel/esimd/memory.hpp Outdated Show resolved Hide resolved

fineg74 added 2 commits December 8, 2023 15:58

Address PR comments

842426c

Merge branch 'accessorCheck3' of https://github.com/fineg74/llvm into…

d8eee69

… accessorCheck3 # Conflicts: # sycl/include/sycl/ext/intel/esimd/memory.hpp

fineg74 temporarily deployed to WindowsCILock December 9, 2023 00:04 — with GitHub Actions Inactive

fineg74 temporarily deployed to WindowsCILock December 9, 2023 00:28 — with GitHub Actions Inactive

Revert some of the last changes

6a74b23

fineg74 temporarily deployed to WindowsCILock December 9, 2023 20:29 — with GitHub Actions Inactive

fineg74 temporarily deployed to WindowsCILock December 9, 2023 20:50 — with GitHub Actions Inactive

sarnex reviewed Dec 13, 2023

View reviewed changes

v-klochkov reviewed Dec 13, 2023

View reviewed changes

sycl/include/sycl/ext/intel/esimd/memory.hpp Outdated Show resolved Hide resolved

Address PR comments

15a25a4

fineg74 had a problem deploying to WindowsCILock December 13, 2023 23:03 — with GitHub Actions Failure

v-klochkov reviewed Dec 13, 2023

View reviewed changes

sycl/include/sycl/ext/intel/esimd/memory.hpp Outdated Show resolved Hide resolved

v-klochkov previously approved these changes Dec 13, 2023

View reviewed changes

fineg74 had a problem deploying to WindowsCILock December 13, 2023 23:24 — with GitHub Actions Error

Fix build break

c60e0a0

fineg74 temporarily deployed to WindowsCILock December 13, 2023 23:43 — with GitHub Actions Inactive

fineg74 had a problem deploying to WindowsCILock December 14, 2023 00:09 — with GitHub Actions Error

Address PR comments

0e4131c

fineg74 temporarily deployed to WindowsCILock December 14, 2023 00:18 — with GitHub Actions Inactive

fineg74 temporarily deployed to WindowsCILock December 14, 2023 00:39 — with GitHub Actions Inactive

v-klochkov approved these changes Dec 14, 2023

View reviewed changes

v-klochkov merged commit ba44e2e into intel:sycl Dec 14, 2023

fineg74 deleted the accessorCheck3 branch December 14, 2023 02:23

[SYCL][ESIMD] Add more stringent compile time checks to atomic_update API #11683

[SYCL][ESIMD] Add more stringent compile time checks to atomic_update API #11683

Uh oh!

Conversation

fineg74 commented Oct 27, 2023

Uh oh!

Uh oh!

Uh oh!

v-klochkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sarnex Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

v-klochkov Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

v-klochkov Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

sarnex Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

fineg74 Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

sarnex Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

fineg74 Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

v-klochkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!