Skip to content

[SYCL] Implement sub-group mask extension #4481

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 16 commits into from
Sep 18, 2021

Conversation

vladimirlaz
Copy link
Contributor

@vladimirlaz vladimirlaz commented Sep 3, 2021

@vladimirlaz vladimirlaz marked this pull request as ready for review September 6, 2021 08:07
@vladimirlaz vladimirlaz requested review from a team as code owners September 6, 2021 08:07
dm-vodopyanov
dm-vodopyanov previously approved these changes Sep 6, 2021
Copy link
Contributor

@dm-vodopyanov dm-vodopyanov left a comment

Choose a reason for hiding this comment

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

SYCL RT changed LGTM

Move Group Mask test to extension folder
Add test for extension macro
@vladimirlaz
Copy link
Contributor Author

@intel/dpcpp-specification-reviewers, could you please have a look?

@gmlueck
Copy link
Contributor

gmlueck commented Sep 8, 2021

@intel/dpcpp-specification-reviewers, could you please have a look?

I added some comments just about the changes to the spec. It would be better to wait for a review from @Pennycook, though. He was involved more closely with this API, and he would be better to review the implementation. John is on vacation this week, returning on Monday.

@vladimirlaz
Copy link
Contributor Author

@gmlueck, @rolandschulz, @Pennycook, could you please have a look on recent changes.
main changes:

  • Change default value for max_bits to 32 (compiler flag will be added later)
  • Remove marray<> from internal layout (leave for insert_bits and extract_bits). Use uint32_t to store bits.
  • Make value for insert_bits(const &T in) and extract_bits(const &T out) deducible by putting in/out to function arguments.
  • Rename to sub_group_mask.
  • Update spec acordingly.

vladimirlaz added a commit to vladimirlaz/llvm-test-suite that referenced this pull request Sep 15, 2021
The test is updated according spec changes in intel/llvm#4481
@Pennycook
Copy link
Contributor

I think there are a few spec updates missing. GitHub won't let me comment on the diff for some reason, so doing so here:

The predefined macro section still says SYCL_EXT_ONEAPI_GROUP_MASK, whereas it should say SYCL_EXT_ONEAPI_SUB_GROUP_MASK after the renaming.

I thought we'd decided that we'd use marray<char, max_bits/CHAR_BIT> instead of marray<uint32_t, ...> for large masks, with a note that max_bits must be a multiple of CHAR_BIT. That would allow you to get rid of marray_size. If max_bits is small enough that you don't need an marray at all, the specification should allow for that too.

@vladimirlaz
Copy link
Contributor Author

I think there are a few spec updates missing. GitHub won't let me comment on the diff for some reason, so doing so here:

The predefined macro section still says SYCL_EXT_ONEAPI_GROUP_MASK, whereas it should say SYCL_EXT_ONEAPI_SUB_GROUP_MASK after the renaming.

I thought we'd decided that we'd use marray<char, max_bits/CHAR_BIT> instead of marray<uint32_t, ...> for large masks, with a note that max_bits must be a multiple of CHAR_BIT. That would allow you to get rid of marray_size. If max_bits is small enough that you don't need an marray at all, the specification should allow for that too.

@Pennycook It looks like I forgot to upload the commits.

  • SYCL_EXT_ONEAPI_GROUP_MASK has been renamed:
  • I replaced marray<> with uint32_t as we have max_bits set to 32.
  • I have removed marray_size from the sources and will remove from the doc (forgot it)

@vladimirlaz vladimirlaz changed the title [SYCL] Implement GroupMask extension [SYCL] Implement sub-group mask extension Sep 16, 2021
@vladimirlaz
Copy link
Contributor Author

vladimirlaz commented Sep 17, 2021

@Pennycook, @gmlueck, @dm-vodopyanov, @rolandschulz could you please review/approve the PR?

@Pennycook
Copy link
Contributor

@Pennycook It looks like I forgot to upload the commits.

  • SYCL_EXT_ONEAPI_GROUP_MASK has been renamed:

There's still one instance of SYCL_EXT_ONEAPI_GROUP_MASK (Line 54 of the extension documentation). Otherwise LGTM.

@vladimirlaz
Copy link
Contributor Author

@Pennycook It looks like I forgot to upload the commits.

  • SYCL_EXT_ONEAPI_GROUP_MASK has been renamed:

There's still one instance of SYCL_EXT_ONEAPI_GROUP_MASK (Line 54 of the extension documentation). Otherwise LGTM.

@Pennycook, I have fixed that. Could you please approve the PR?

@bader
Copy link
Contributor

bader commented Sep 18, 2021

The specification is available under https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/SubGroupMask/SubGroupMask.asciidoc

@vladimirlaz, please, update the link to the specification.

@vladimirlaz
Copy link
Contributor Author

The specification is available under https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/SubGroupMask/SubGroupMask.asciidoc

@vladimirlaz, please, update the link to the specification.

the spec is moved in the scope of the PR. The comment contains the new location.

@vladimirlaz vladimirlaz merged commit 78a3e77 into intel:sycl Sep 18, 2021
@vladimirlaz vladimirlaz deleted the group_mask branch October 8, 2021 06:18
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.

6 participants