Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SYCL][ESIMD]Implement slm_scatter accepting compile time properties #12591

Merged
merged 8 commits into from
Feb 7, 2024

Conversation

fineg74
Copy link
Contributor

@fineg74 fineg74 commented Feb 2, 2024

This implements the unified memory API for slm_scatter with local accessors

@sarnex
Copy link
Contributor

sarnex commented Feb 2, 2024

Looks like the tests are failing, I'll hold my review for now

@v-klochkov
Copy link
Contributor

Looks like the tests are failing, I'll hold my review for now

@fineg74 for such big code-changes please create 'Draft PR' first, then do a series of extra changes if necessary to get passes, then change draft to real PR.

@fineg74 fineg74 marked this pull request as draft February 2, 2024 17:35
@fineg74 fineg74 marked this pull request as ready for review February 2, 2024 21:29
Copy link
Contributor

@v-klochkov v-klochkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I finished the 1st scan/review-pass. Please see my comments.

sycl/include/sycl/ext/intel/esimd/memory.hpp Outdated Show resolved Hide resolved
sycl/include/sycl/ext/intel/esimd/memory.hpp Outdated Show resolved Hide resolved
sycl/include/sycl/ext/intel/esimd/memory.hpp Show resolved Hide resolved
sycl/include/sycl/ext/intel/esimd/memory.hpp Show resolved Hide resolved
sycl/test/esimd/memory_properties.cpp Outdated Show resolved Hide resolved
@v-klochkov
Copy link
Contributor

Looks like the tests are failing, I'll hold my review for now

@sarnex - CI is green, please finish your code-review

@sarnex
Copy link
Contributor

sarnex commented Feb 7, 2024

Will do, sorry

/// void slm_scatter(
/// OffsetSimdViewT byte_offsets, simd<T, N> vals,
/// PropertyListT props = {}); // (slm-sc-4)
/// Loads ("gathers") elements of the type 'T' from Shared Local Memory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment seems wrong, should be for scatter

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address in follow-up PR for local accessors and scatter

@v-klochkov v-klochkov merged commit 9617939 into intel:sycl Feb 7, 2024
12 checks passed
@fineg74 fineg74 deleted the slm_scatter branch February 7, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants