[SYCL][ESIMD] Implement unified memory API - block_store slm and local accessors #11921

sarnex · 2023-11-16T22:09:48Z

This change implements the last piece for block_store: local accessors and SLM.

turinevgeny

lgtm.

v-klochkov

Thank you for the fix. I finished the 1st code-review pass.

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

sycl/test-e2e/ESIMD/unified_memory_api/Inputs/block_store.hpp

sycl/test/esimd/memory_properties.cpp

v-klochkov

Mark it as 'request changes' to avoid accidental merge before fixing it.

sarnex · 2023-11-20T19:07:43Z

@v-klochkov Thanks a lot for the review, all feedback should be addressed in the latest commit

…l accessors Signed-off-by: Sarnie, Nick <[email protected]>

Signed-off-by: Sarnie, Nick <[email protected]>

sarnex · 2023-11-22T20:48:43Z

Sorry for force-push, rebasing after pulldown

v-klochkov · 2023-11-23T02:44:16Z

Sorry for force-push, rebasing after pulldown

Why not to use git merge intel_llvm/sycl where intel_llvm points to https://github.com/intel/llvm ?
It would pull all the `pulldown' changes to your branch.

v-klochkov

few nits + 1 comment regarding driver version requirement in the tests.

v-klochkov · 2023-11-23T03:02:27Z

sycl/test-e2e/ESIMD/unified_memory_api/Inputs/block_store.hpp

           else
-             slm_block_store(LocalElemOffset, Vals);
+             slm_block_store<T, N>(LocalElemOffset, Vals);


nit: IMO, it would be better to not pass <T, N>, which would make the test closer to what user is supposed to do and make the call less verbose and make the check stronger.

did this for all cases besides simd_view, we need to specify there because the compiler can't autodeduce, and i double checked to make sure the template parameters are the same as existing apis, so i think this is just a simd_view thing

sycl/test-e2e/ESIMD/unified_memory_api/Inputs/block_store.hpp

sycl/test-e2e/ESIMD/unified_memory_api/block_store_slm_acc.cpp

sarnex · 2023-11-27T16:30:29Z

Why not to use git merge intel_llvm/sycl

For some reason I thought it would list all of the new unrelated changes in the PR making it hard to view, but looks like I am wrong and I will use merge in the future, thanks!

Signed-off-by: Sarnie, Nick <[email protected]>

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

v-klochkov · 2023-11-28T16:09:47Z

1 test failed. The driver version in CI looks very old.
Two extra changes may be needed:

add driver version check. If use isGPUDriverGE(),then that function may need some tuning. The driver version for level_zero started showing up in linux format (and the other format for open_cl)
Limit the amount of output in verify function by 32 lines per each failed case.

sarnex · 2023-11-28T16:11:49Z

@v-klochkov Ah ok it's only windows. Will do both suggestions, thanks

Signed-off-by: Sarnie, Nick <[email protected]>

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

v-klochkov

Temporarily blocking to resolve the question with simd_view parameters of slm_block_store()

Signed-off-by: Sarnie, Nick <[email protected]>

againull · 2023-12-01T19:12:17Z

Seems that SYCL :: ESIMD/unified_memory_api/block_store_slm_acc.cpp is failing in post-commit after this PR, could you please take a look:
https://github.com/intel/llvm/actions/runs/7053606441/job/19201224909

sarnex · 2023-12-01T19:13:54Z

Looking now

sarnex · 2023-12-01T19:47:55Z

Fix: #12054

againull · 2023-12-01T20:19:24Z

Fix: #12054

Thanks a lot for the quick fix!

…e_slm_acc.cpp (#12054) Originally it seemed the problem was only on Windows, but it seems it happens on Linux too, see [here](#11921 (comment)). I manually verified it fails consistently before this version and passes consistently after this version. Signed-off-by: Sarnie, Nick <[email protected]>

sarnex temporarily deployed to WindowsCILock November 16, 2023 23:31 — with GitHub Actions Inactive

sarnex had a problem deploying to WindowsCILock November 17, 2023 00:31 — with GitHub Actions Failure

sarnex force-pushed the blockslm branch from e832c8d to c38e1a1 Compare November 17, 2023 14:51

sarnex marked this pull request as ready for review November 17, 2023 16:24

sarnex requested a review from a team as a code owner November 17, 2023 16:24

sarnex temporarily deployed to WindowsCILock November 17, 2023 20:23 — with GitHub Actions Inactive

sarnex had a problem deploying to WindowsCILock November 17, 2023 23:45 — with GitHub Actions Failure

turinevgeny approved these changes Nov 18, 2023

View reviewed changes

v-klochkov reviewed Nov 18, 2023

View reviewed changes

v-klochkov requested changes Nov 18, 2023

View reviewed changes

sarnex temporarily deployed to WindowsCILock November 20, 2023 20:26 — with GitHub Actions Inactive

sarnex temporarily deployed to WindowsCILock November 20, 2023 22:21 — with GitHub Actions Inactive

sarnex requested a review from v-klochkov November 22, 2023 18:23

sarnex added 4 commits November 22, 2023 12:48

[SYCL][ESIMD] Implement unified memory API - block_store slm and loca…

26529d7

…l accessors Signed-off-by: Sarnie, Nick <[email protected]>

whitespace

e09975f

Signed-off-by: Sarnie, Nick <[email protected]>

whitespace 2

da83991

Signed-off-by: Sarnie, Nick <[email protected]>

address feedback

5e241ee

Signed-off-by: Sarnie, Nick <[email protected]>

sarnex force-pushed the blockslm branch from 8365a21 to 5e241ee Compare November 22, 2023 20:48

v-klochkov approved these changes Nov 23, 2023

View reviewed changes

sarnex temporarily deployed to WindowsCILock November 23, 2023 08:06 — with GitHub Actions Inactive

sarnex temporarily deployed to WindowsCILock November 23, 2023 12:03 — with GitHub Actions Inactive

sarnex added 2 commits November 27, 2023 09:18

address feedback 2

6989fdf

Signed-off-by: Sarnie, Nick <[email protected]>

missed call

d7e07a2

Signed-off-by: Sarnie, Nick <[email protected]>

v-klochkov reviewed Nov 27, 2023

View reviewed changes

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

sarnex temporarily deployed to WindowsCILock November 27, 2023 20:24 — with GitHub Actions Inactive

sarnex had a problem deploying to WindowsCILock November 27, 2023 21:10 — with GitHub Actions Failure

fix win failure and limit output

a40009d

Signed-off-by: Sarnie, Nick <[email protected]>

sarnex temporarily deployed to WindowsCILock November 28, 2023 20:59 — with GitHub Actions Inactive

sarnex temporarily deployed to WindowsCILock November 28, 2023 21:27 — with GitHub Actions Inactive

v-klochkov reviewed Nov 28, 2023

View reviewed changes

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

v-klochkov requested changes Nov 28, 2023

View reviewed changes

remove simd_view overloads

19bc0c1

Signed-off-by: Sarnie, Nick <[email protected]>

sarnex had a problem deploying to WindowsCILock November 29, 2023 21:03 — with GitHub Actions Error

fix last commit

34e5e1c

Signed-off-by: Sarnie, Nick <[email protected]>

sarnex temporarily deployed to WindowsCILock November 29, 2023 21:09 — with GitHub Actions Inactive

sarnex temporarily deployed to WindowsCILock November 29, 2023 21:31 — with GitHub Actions Inactive

Merge remote-tracking branch 'upstream/sycl' into blockslm

3fe780f

sarnex temporarily deployed to WindowsCILock November 30, 2023 16:13 — with GitHub Actions Inactive

sarnex temporarily deployed to WindowsCILock November 30, 2023 16:48 — with GitHub Actions Inactive

v-klochkov approved these changes Nov 30, 2023

View reviewed changes

v-klochkov merged commit 646ab08 into intel:sycl Nov 30, 2023

sarnex mentioned this pull request Dec 1, 2023

[SYCL][ESIMD][E2E] Add Linux driver version requirement to block_store_slm_acc.cpp #12054

Merged

[SYCL][ESIMD] Implement unified memory API - block_store slm and local accessors #11921

[SYCL][ESIMD] Implement unified memory API - block_store slm and local accessors #11921

Uh oh!

Conversation

sarnex commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

turinevgeny left a comment

Choose a reason for hiding this comment

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!

Uh oh!

v-klochkov left a comment

Choose a reason for hiding this comment

Uh oh!

sarnex commented Nov 20, 2023

Uh oh!

sarnex commented Nov 22, 2023

Uh oh!

v-klochkov commented Nov 23, 2023

Uh oh!

v-klochkov left a comment

Choose a reason for hiding this comment

Uh oh!

v-klochkov Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

sarnex Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sarnex commented Nov 27, 2023

Uh oh!

Uh oh!

v-klochkov commented Nov 28, 2023

Uh oh!

sarnex commented Nov 28, 2023

Uh oh!

Uh oh!

v-klochkov left a comment

Choose a reason for hiding this comment

Uh oh!

againull commented Dec 1, 2023

Uh oh!

sarnex commented Dec 1, 2023

Uh oh!

sarnex commented Dec 1, 2023

Uh oh!

againull commented Dec 1, 2023

Uh oh!

Uh oh!

sarnex commented Nov 16, 2023 •

edited

Loading