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][Docs][Joint matrix] Add overloads and restrictions for the offset load store #15499

Merged
merged 4 commits into from
Oct 22, 2024

Conversation

dkhaldi
Copy link
Contributor

@dkhaldi dkhaldi commented Sep 24, 2024

  • Add missing restriction on the stride of the checked variants of load/store
  • Add new overloads of joint_matrix_load and joint_matrix_store where the offsets are separated from the base pointer and added as separate arguments. I kept the same name as the expectation is to remove the regular variants once the new ones are used instead.
  • Add restrictions on both the regular and the offset joint_matrix_load/store on PVC since in the current implementation, no runtime checks are added as they are expensive. The fall back to 1d load/store is done using a flag instead.

@dkhaldi dkhaldi requested a review from a team as a code owner September 24, 2024 19:56
@@ -148,28 +148,59 @@ template <typename Group, typename T, size_t Rows, size_t Cols,
access::decorated IsDecorated>
void joint_matrix_store(Group g,
const joint_matrix<Group, T, use::a, Rows, Cols, Layout> &res,
multi_ptr<T, Space, IsDecorated> dest, size_t stride);
multi_ptr<T, Space, IsDecorated> dest, size_t Stride);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why capitalize this parameter name? All the other parameter names start with a lower case letter. Our style is that function parameter names are lower case (snake_case) while template parameter names are upper case (CamelCase).

I see below that you have added parameter names RowIndex and ColIndex. These should be row_index and col_index to be consistent.


- The `Stride` argument to `joint_matrix_load` and
`joint_matrix_store` must be a multiple of 8 bytes. Also, `Stride`
should not exceed `2^24^` bytes.
Copy link
Contributor

Choose a reason for hiding this comment

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

The stride parameter is the number of elements, not the number of bytes. It would be better to reword this like:

The stride parameter to joint_matrix_load and joint_matrix_store has the following restrictions:

  • The value stride * sizeof(T1) must be a multiple of 8, and
  • The value of stride * sizeof(T1) must not exceed 224.

@@ -462,6 +493,9 @@ The checked APIs are currently available in devices with the architecture
`architecture::intel_gpu_pvc`. The following restrictions apply to
these checked APIs:

- The `Stride` argument must be a multiple of 8 bytes. Also, `Stride`
should not exceed `2^24^` bytes.
Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment in the other file about the wording of this restriction.

@dkhaldi dkhaldi requested a review from gmlueck October 10, 2024 18:24
@dkhaldi dkhaldi requested review from gmlueck and removed request for gmlueck October 18, 2024 01:28
@dkhaldi
Copy link
Contributor Author

dkhaldi commented Oct 21, 2024

@intel/llvm-gatekeepers, please help merge this.

@steffenlarsen steffenlarsen merged commit 475ca2d into intel:sycl Oct 22, 2024
2 checks passed
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