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 gather(lacc) accepting compile time properties #12533

Merged
merged 5 commits into from
Feb 5, 2024

Conversation

fineg74
Copy link
Contributor

@fineg74 fineg74 commented Jan 30, 2024

This implements the unified memory API for gather with local accessors

# Conflicts:
#	sycl/test/esimd/memory_properties.cpp
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.

Looks really good! I cannot add anything except fixing 1 letter ))
@sarnex - please also take a look, I could miss something.

Assume approved, but pending to review/approval from Nick. Thank you.

sycl/test-e2e/ESIMD/unified_memory_api/Inputs/gather.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

Really sorry, I promised I'd do this yesterday but something came up.

LGTM with Slava's comments

/// simd<T, N> gather(AccessorT acc, simd<uint32_t, N / VS> byte_offsets,
/// simd_mask<N / VS> mask,
/// PropertyListT props = {}); // (lacc-ga-2)
/// Supported platforms: DG2, PVC in most cases. The DG2/PVC is not required if
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Supported platforms: DG2, PVC in most cases. The DG2/PVC is not required if
/// Supported platforms: DG2, PVC in most cases. DG2/PVC is not required if

Nit, same thing in a few places

/// simd_mask<N> mask, simd<T, N> pass_thru,
/// PropertyListT props = {}); // (lacc-ga-4)
/// This function is identical to (lacc-ga-1) except that vector size is fixed
/// to 1. This variant is added for convenience and let user omit the template
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// to 1. This variant is added for convenience and let user omit the template
/// to 1. This variant is added for convenience and lets the user omit the template

same thing in a few places, just a nit

///
/// The next 3 functions are similar to (lacc-ga-1,2,3), but they don't have
/// the template parameter 'VS'. These functions are added for convenience and
/// to make it possible for user to omit the template parameters T and N,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// to make it possible for user to omit the template parameters T and N,
/// to make it possible for the user to omit the template parameters T and N,

Co-authored-by: Vyacheslav Klochkov <vyacheslav.n.klochkov@intel.com>
@v-klochkov v-klochkov merged commit d286f4a into intel:sycl Feb 5, 2024
12 checks passed
@fineg74 fineg74 deleted the local_accessors branch February 5, 2024 20:10
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