Skip to content

[SYCL] Implement get_multi_ptr for accessor and local_accessor #8249

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

Merged
merged 17 commits into from
Feb 27, 2023

Conversation

mmoadeli
Copy link
Contributor

@mmoadeli mmoadeli commented Feb 8, 2023

  • Implement get_multi_ptr for accessor and local_accessor
  • Add test for get_multi_ptr
  • Update the affected default ctor test

@mmoadeli mmoadeli requested a review from a team as a code owner February 8, 2023 13:11
@mmoadeli mmoadeli temporarily deployed to aws February 8, 2023 14:33 — with GitHub Actions Inactive
@mmoadeli mmoadeli temporarily deployed to aws February 8, 2023 17:25 — with GitHub Actions Inactive
@mmoadeli mmoadeli temporarily deployed to aws February 9, 2023 07:52 — with GitHub Actions Inactive
@@ -0,0 +1,46 @@
// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple %s -o %t.out
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
// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple %s -o %t.out
// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple %s -fsyntax-only

See some BKMs on tests: here

Since the test only contains a few static_asserts which do most of the actual checking, I would also suggest to maybe refactor the test to significantly simplify it. We can turn everything into a single function, which accepts handler and buffer, creates accessors and then checks return types of methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @AlexeySachkov, done.

@AlexeySachkov AlexeySachkov requested a review from a team February 21, 2023 16:23
@mmoadeli mmoadeli temporarily deployed to aws February 21, 2023 17:28 — with GitHub Actions Inactive
@mmoadeli mmoadeli temporarily deployed to aws February 21, 2023 18:13 — with GitHub Actions Inactive
// TODO: uncomment check with get_multi_ptr() when SYCL 2020 mupti_ptr feature
// will be merged
// assert(B.get_multi_ptr() == nullptr);
assert(B.get_multi_ptr<sycl::access::decorated::yes>() == nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

For good measure, could you please add a sycl::access:decorated::no and sycl::access::decorated::legacy test case here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @steffenlarsen , done.

@mmoadeli mmoadeli temporarily deployed to aws February 25, 2023 23:11 — with GitHub Actions Inactive
@mmoadeli mmoadeli temporarily deployed to aws February 25, 2023 23:44 — with GitHub Actions Inactive
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

LGTM!

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