Skip to content

[SYCL] Update get_pointer to return T* for target::device specialized accessor. #8874

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 77 commits into from
May 18, 2023
Merged

[SYCL] Update get_pointer to return T* for target::device specialized accessor. #8874

merged 77 commits into from
May 18, 2023

Conversation

mmoadeli
Copy link
Contributor

@mmoadeli mmoadeli commented Mar 30, 2023 •
edited
Loading

  • Update get_pointer to return T* for target::device specialized accessor according to the Specification
  • Declare src of async_work_group_copy in group.hpp and nd_item.hpp as global_ptr<const>
  • Implement implicit conversion from multi_ptr<T> to multi_ptr<const T>
  • Modifies is_native_op to cover both const and non-const types.

@mmoadeli mmoadeli requested review from a team as code owners March 30, 2023 11:09
@mmoadeli mmoadeli requested a review from sergey-semenov March 30, 2023 11:09
@mmoadeli mmoadeli marked this pull request as draft March 30, 2023 11:18
@mmoadeli mmoadeli marked this pull request as ready for review March 30, 2023 12:51
@mmoadeli mmoadeli temporarily deployed to aws March 30, 2023 13:08 — with GitHub Actions Inactive
@mmoadeli mmoadeli temporarily deployed to aws March 30, 2023 13:26 — with GitHub Actions Inactive
@mmoadeli mmoadeli temporarily deployed to aws March 30, 2023 14:37 — with GitHub Actions Inactive
@mmoadeli mmoadeli temporarily deployed to aws March 30, 2023 17:03 — with GitHub Actions Inactive
@mmoadeli mmoadeli temporarily deployed to aws March 31, 2023 12:33 — with GitHub Actions Inactive
@mmoadeli mmoadeli temporarily deployed to aws March 31, 2023 12:43 — with GitHub Actions Inactive
@mmoadeli mmoadeli temporarily deployed to aws March 31, 2023 13:18 — with GitHub Actions Inactive
@mmoadeli mmoadeli temporarily deployed to aws March 31, 2023 13:51 — with GitHub Actions Inactive
@mmoadeli mmoadeli temporarily deployed to aws April 1, 2023 04:10 — with GitHub Actions Inactive
@mmoadeli mmoadeli temporarily deployed to aws April 1, 2023 05:55 — with GitHub Actions Inactive
@mmoadeli
The return type of get_pointer in mode::read is const T*. This comes …
7d1bf5a 
…from definition of the value_type in mode_read, which defines the type as a const of the accessor type.

In a number of situations including the one involved in this PR and creating multi_ptr from the accessor of the initial type, this lead to an error for not being able to cast from const to non const pointer type.
@mmoadeli mmoadeli temporarily deployed to aws April 2, 2023 18:28 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen mentioned this pull request May 17, 2023
Closed
@steffenlarsen steffenlarsen requested a review from victor-eds May 17, 2023 09:40
@mmoadeli mmoadeli temporarily deployed to aws May 17, 2023 10:02 — with GitHub Actions Inactive
@mmoadeli mmoadeli temporarily deployed to aws May 17, 2023 10:47 — with GitHub Actions Inactive
Naghasan
Naghasan requested changes May 17, 2023
View reviewed changes
sycl/include/sycl/multi_ptr.hpp Outdated
Comment on lines 189 to 190
: multi_ptr(detail::cast_AS<decorated_type *>(
Accessor.template get_multi_ptr<DecorateAddress>().get())) {}
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
: multi_ptr(detail::cast_AS<decorated_type *>(
Accessor.template get_multi_ptr<DecorateAddress>().get())) {}
: multi_ptr(Accessor.template get_multi_ptr<DecorateAddress>()) {}

that's likely to insert an unnecessary cast otherwise

Copy link
Contributor Author

@mmoadeli mmoadeli May 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @Naghasan. I addressed un-necessary casting using get_decorated. Removing get as suggested failed to work for a number of combinations of address_space and decorated values.

@Naghasan Naghasan mentioned this pull request May 17, 2023
Open
@JackAKirk JackAKirk mentioned this pull request May 17, 2023
Closed
@mmoadeli mmoadeli requested a review from Naghasan May 18, 2023 01:11
@mmoadeli mmoadeli temporarily deployed to aws May 18, 2023 01:28 — with GitHub Actions Inactive
@mmoadeli mmoadeli temporarily deployed to aws May 18, 2023 02:00 — with GitHub Actions Inactive
@bader
Copy link
Contributor

bader commented May 18, 2023

@mmoadeli, please, resolve merge conflicts.
If pre-commit checks pass after the merge, please, ask @intel/llvm-gatekeepers team to merge.

@mmoadeli mmoadeli temporarily deployed to aws May 18, 2023 20:46 — with GitHub Actions Inactive
@mmoadeli mmoadeli temporarily deployed to aws May 18, 2023 21:26 — with GitHub Actions Inactive
@mmoadeli
Copy link
Contributor Author

mmoadeli commented May 18, 2023 •
edited
Loading

@intel/llvm-gatekeepers friendly request to merge this PR please

@aelovikov-intel aelovikov-intel merged commit 712cb4e into intel:sycl May 18, 2023
whitneywhtsang added a commit to whitneywhtsang/llvm that referenced this pull request May 19, 2023
@whitneywhtsang
Revert "[SYCL] Update get_pointer to return T* for target::device spe…
7fb1a0d 
…cialized accessor. (intel#8874)"

This reverts commit 712cb4e.
This was referenced Jun 5, 2023
Closed
Closed
yubingex007-a11y added a commit to yubingex007-a11y/llvm that referenced this pull request Jun 12, 2023
@yubingex007-a11y
[SYCL][Matrix] Update element_wise_all_sizes_impl.hpp according to in…
f7cd66b 
…tel#8874
steffenlarsen pushed a commit that referenced this pull request Jun 12, 2023
@yubingex007-a11y
[SYCL][Matrix] Update element_wise_all_sizes_impl.hpp according to #8874
395aa8a 
 (#9822)
@vpirogov vpirogov mentioned this pull request Jun 13, 2023
Closed
@mmoadeli mmoadeli deleted the get_pointer_device branch July 7, 2023 10:39
dm-vodopyanov added a commit to dm-vodopyanov/llvm that referenced this pull request Jul 10, 2023
@dm-vodopyanov
[SYCL] Update some SYCL FPGA tests after intel#8874
6a5d924 
This patch fixes compilation error in fpga_lsu.cpp and
fpga_latency_control_lsu.cpp. Tested locally with open-source and
closed-source compilers.
Currently there is no external testing with the accelerator device in
open source.
aelovikov-intel pushed a commit that referenced this pull request Jul 11, 2023
@dm-vodopyanov
[SYCL] Update some SYCL FPGA tests after #8874 (#10284)
1531fa1 
This patch fixes compilation error in fpga_lsu.cpp and
fpga_latency_control_lsu.cpp. Tested locally with open-source and
closed-source compilers.
Currently there is no external testing with the accelerator device in
open source.
@shuoniu-intel shuoniu-intel mentioned this pull request Aug 2, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmlueck gmlueck gmlueck left review comments

@Naghasan Naghasan Naghasan approved these changes

@v-klochkov v-klochkov v-klochkov approved these changes

@mdtoguchi mdtoguchi mdtoguchi approved these changes

@steffenlarsen steffenlarsen steffenlarsen approved these changes

@sergey-semenov sergey-semenov Awaiting requested review from sergey-semenov sergey-semenov is a code owner automatically assigned from intel/llvm-reviewers-runtime

@sommerlukas sommerlukas Awaiting requested review from sommerlukas

@victor-eds victor-eds Awaiting requested review from victor-eds

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mmoadeli @steffenlarsen @bader @gmlueck @Naghasan @victor-eds @v-klochkov @mdtoguchi @aelovikov-intel