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.

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

[SYCL][DOC] Update dot_acc extension spec #10113

Draft
wants to merge 5 commits into
base: sycl
Choose a base branch
from
Draft

[SYCL][DOC] Update dot_acc extension spec #10113

wants to merge 5 commits into from

Conversation

gmlueck
Copy link
Contributor

@gmlueck gmlueck commented Jun 28, 2023 •
edited
Loading

Update the sycl_ext_oneapi_dot_accumulate extension spec to:

  • Use the latest specification template.
  • Document the "packed" APIs. These were previously shown in the "sample header" section, but there was no description.
  • Deprecate the old "packed" overloads and introduce new overloads with more descriptive names like doc_acc_4x8packed_su. This new names are similar to the OpenCL C naming style for the equivalent functions.
  • Deprecate the API that takes two unsigned vectors and a signed accumulator. Replace this with an API that takes two unsigned vectors and an unsigned accumulator. This matches the SPIR-V extension and also the OpenCL API.

@gmlueck
[SYCL][DOC] Update dot_acc extension spec
2365dc8 
Update the sycl_ext_oneapi_dot_accumulate extension spec to:

* Use the latest specification template.
* Document the "packed" APIs.  These were previously shown in the
  "sample header" section, but there was no description.
* Deprecate the old "packed" overloads and introduce new overloads
  with the more descriptive name `doc_acc_4x8packed`.  This new name
  is consistent with the OpenCL C naming style for similar functions.
@gmlueck gmlueck requested a review from a team as a code owner June 28, 2023 14:00
@gmlueck gmlueck requested a review from bashbaug June 28, 2023 14:01
bashbaug
bashbaug approved these changes Jul 5, 2023
View reviewed changes
Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, this is a nice improvement.

No blockers but here are a few comments to consider.

sycl/doc/extensions/supported/sycl_ext_oneapi_dot_accumulate.asciidoc Outdated
Comment on lines 85 to 93
int32_t dot_acc(vec<int8_t,4> a, vec<int8_t,4> b, int32_t c);
int32_t dot_acc(vec<int8_t,4> a, vec<uint8_t,4> b, int32_t c);
int32_t dot_acc(vec<uint8_t,4> a, vec<int8_t,4> b, int32_t c);
int32_t dot_acc(vec<uint8_t,4> a, vec<uint8_t,4> b, int32_t c);

=== Add to Section 4.13.6 - Geometric Functions
int32_t doc_acc_4x8packed(int32_t a, int32_t b, int32_t c);
int32_t doc_acc_4x8packed(int32_t a, uint32_t b, int32_t c);
int32_t doc_acc_4x8packed(uint32_t a, int32_t b, int32_t c);
int32_t doc_acc_4x8packed(uint32_t a, uint32_t b, int32_t c);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a terribly strong opinion about either of these, but noting for completeness: If we want to align even closer to the related OpenCL extension we should consider the following changes:

  1. The OpenCL extension only has dot_acc functions that do the dot product and accumulate for the variants that saturate, otherwise the function is simply dot, which does a dot product.
  2. In addition to the return value difference, the type for the packed a and b arguments is unconditionally an unsigned integer in the OpenCL extension. To disambiguate the different types the signedness of the argument is encoded in the function name (the return type is also), so for example if the packed a is signed and b is unsigned the full function is int dot_4x8packed_su_int(uint a, uint b).

Links for reference:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the suggestion to use uint32_t always for the "packed" parameters, so I also adopted the "_ss" (etc.) convention. Done in 84f1ff8.

It didn't make sense to me to adopt the final "_int" / "_uint" signifying the return type for the packed APIs unless we also adopt that convention for the unpacked APIs. I decided to leave it off for both.

I don't know why this extension added dot_acc rather than just dot. Dropping the "accumulate" operation seemed like a bigger change, so I decided not to do that here.

sycl/doc/extensions/supported/sycl_ext_oneapi_dot_accumulate.asciidoc
Comment on lines +73 to +74
|1
|Initial version of this extension.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to call this version one and the initial version? IMHO this loses history we might want to preserve. If we do call this version one would it be better to add it as a new document and move the existing document to a "deprecated" or "removed" directory so we don't lose track of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an API version, not a document version. Normally, I would bump the API version since this PR adds new APIs. However, we never implemented any API versioning prior to this PR, and I can't go back in time and add it now. The best I can do is to start versioning at "1" now.

Regarding the loss of history ... this PR doesn't remove any of the old APIs. They are still retained as "deprecated", and this extension document still describes them. (In fact, they are described better now than in the previous document.)

@gmlueck
Address review feedback
84f1ff8 
* Deprecate the overload taking two unsigned vectors and returning a
  signed result and replace it with a version returning an unsigned
  result.  Addresses the open issue.

* Change the names of the new "packed" APIs to include a suffix which
  tells how the `a` and `b` vectors are interpreted (signed vs.
  unsigned).

* Change the new "packed" APIs so that `a` and `b` are always unsigned
  integers.  The name of the function now tells how to interpret them.
@gmlueck
Copy link
Contributor Author

gmlueck commented Jul 27, 2023

@intel/dpcpp-specification-reviewers this is ready for re-review now.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 1, 2024

This pull request is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be automatically closed in 30 days.

@github-actions github-actions bot added the Stale label Feb 1, 2024
@gmlueck gmlueck removed the Stale label Feb 13, 2024
@github-actions GitHub Actions
Copy link
Contributor

This pull request is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be automatically closed in 30 days.

@github-actions github-actions bot added the Stale label Sep 15, 2024
Pennycook
Pennycook requested changes Oct 8, 2024
View reviewed changes
Copy link
Contributor

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

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

Spotted a typo, otherwise LGTM.

sycl/doc/extensions/supported/sycl_ext_oneapi_dot_accumulate.asciidoc Outdated
Comment on lines 90 to 93
int32_t doc_acc_4x8packed_ss(uint32_t a, uint32_t b, int32_t c);
int32_t doc_acc_4x8packed_su(uint32_t a, uint32_t b, int32_t c);
int32_t doc_acc_4x8packed_us(uint32_t a, uint32_t b, int32_t c);
uint32_t doc_acc_4x8packed_uu(uint32_t a, uint32_t b, uint32_t c);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be:

Suggested change
int32_t doc_acc_4x8packed_ss(uint32_t a, uint32_t b, int32_t c);
int32_t doc_acc_4x8packed_su(uint32_t a, uint32_t b, int32_t c);
int32_t doc_acc_4x8packed_us(uint32_t a, uint32_t b, int32_t c);
uint32_t doc_acc_4x8packed_uu(uint32_t a, uint32_t b, uint32_t c);
int32_t dot_acc_4x8packed_ss(uint32_t a, uint32_t b, int32_t c);
int32_t dot_acc_4x8packed_su(uint32_t a, uint32_t b, int32_t c);
int32_t dot_acc_4x8packed_us(uint32_t a, uint32_t b, int32_t c);
uint32_t dot_acc_4x8packed_uu(uint32_t a, uint32_t b, uint32_t c);

The doc typo appears in a few other places below, too.

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. Fixed in 995e783

@gmlueck gmlueck removed the Stale label Oct 8, 2024
@gmlueck
Minor updates
37ae853 
* Update to single-year copyright format
* Update base SYCL 2020 revision to latest
@gmlueck gmlueck marked this pull request as draft October 9, 2024 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bashbaug bashbaug bashbaug approved these changes

@rdeodhar rdeodhar rdeodhar left review comments

@Pennycook Pennycook Pennycook approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@gmlueck @Pennycook @bashbaug @rdeodhar