Skip to content

[SYCL][Matrix] Add initial get_coord API #7851

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 8 commits into from
Mar 28, 2023

Conversation

arnamoy10
Copy link
Contributor

@arnamoy10 arnamoy10 commented Dec 20, 2022

This patch adds an initial API for the retrieval of coordinates from a work item element. A get_coord() method is added to the intel namespace to work on wi_element class. Also, a relevant SPIRV op is added, which the get_coord() gets lowered to.

This is recreated PR from my forked repo. The discussions are in the original (closed) PR #7037

@dkhaldi
Copy link
Contributor

dkhaldi commented Dec 20, 2022

We recently added support for the unified matrix API. the one you are using now in matrix-jit.hpp is an old API.
The unified API is now in matrix-unified.hpp. But since get_coord is an Intel-specific extension, you should move your development to matrix-intel.hpp.
The examples should also use the new syntax.

@arnamoy10
Copy link
Contributor Author

We recently added support for the unified matrix API. the one you are using now in matrix-jit.hpp is an old API. The unified API is now in matrix-unified.hpp. But since get_coord is an Intel-specific extension, you should move your development to matrix-intel.hpp. The examples should also use the new syntax.

Done with changes.

@arnamoy10 arnamoy10 requested a review from JackAKirk January 28, 2023 12:56
@arnamoy10 arnamoy10 temporarily deployed to aws January 28, 2023 13:20 — with GitHub Actions Inactive
@arnamoy10 arnamoy10 temporarily deployed to aws January 28, 2023 14:01 — with GitHub Actions Inactive
@arnamoy10 arnamoy10 temporarily deployed to aws February 1, 2023 18:48 — with GitHub Actions Inactive
@arnamoy10 arnamoy10 temporarily deployed to aws February 2, 2023 01:40 — with GitHub Actions Inactive
@JackAKirk
Copy link
Contributor

LGTM. Just had a few minor comments.

@arnamoy10 arnamoy10 temporarily deployed to aws February 2, 2023 15:46 — with GitHub Actions Inactive
@arnamoy10 arnamoy10 temporarily deployed to aws February 2, 2023 16:18 — with GitHub Actions Inactive
Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

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

(SPIR-V) Headers are LGTM

@dkhaldi dkhaldi requested a review from Nuullll March 1, 2023 15:22
@arnamoy10 arnamoy10 temporarily deployed to aws March 17, 2023 17:37 — with GitHub Actions Inactive
@arnamoy10
Copy link
Contributor Author

Test-suite PR: intel/llvm-test-suite#1676

@arnamoy10 arnamoy10 temporarily deployed to aws March 22, 2023 19:24 — with GitHub Actions Inactive
@arnamoy10 arnamoy10 temporarily deployed to aws March 22, 2023 20:01 — with GitHub Actions Inactive
@arnamoy10 arnamoy10 temporarily deployed to aws March 23, 2023 12:42 — with GitHub Actions Inactive
@arnamoy10 arnamoy10 temporarily deployed to aws March 23, 2023 13:13 — with GitHub Actions Inactive
@arnamoy10 arnamoy10 temporarily deployed to aws March 23, 2023 15:24 — with GitHub Actions Inactive
@arnamoy10 arnamoy10 temporarily deployed to aws March 23, 2023 15:56 — with GitHub Actions Inactive
Copy link
Contributor

@dkhaldi dkhaldi left a comment

Choose a reason for hiding this comment

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

LGTM.
Minor: remove comment on line 195

@arnamoy10
Copy link
Contributor Author

LGTM. Minor: remove comment on line 195

Done

@arnamoy10 arnamoy10 temporarily deployed to aws March 23, 2023 19:17 — with GitHub Actions Inactive
@arnamoy10
Copy link
Contributor Author

@intel/llvm-reviewers-runtime Please take a look and merge if it looks good. Thanks

@arnamoy10 arnamoy10 temporarily deployed to aws March 23, 2023 21:20 — with GitHub Actions Inactive
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.

@arnamoy10, please, update description with proper wording, e.g., what this patch introduces/etc

@arnamoy10
Copy link
Contributor Author

@arnamoy10, please, update description with proper wording, e.g., what this patch introduces/etc

Updated, thank you.

@arnamoy10
Copy link
Contributor Author

@intel/llvm-gatekeepers , help merge this patch please?

@dm-vodopyanov
Copy link
Contributor

dm-vodopyanov commented Mar 28, 2023

Failed Tests (1):
SYCL :: Basic/kernel_info.cpp

Unrelated to this patch. Issue: #8766

@dm-vodopyanov dm-vodopyanov merged commit afebb25 into intel:sycl Mar 28, 2023
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