Skip to content

[SYCL][Fusion] Add HIP support #11003

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 24 commits into from
Oct 25, 2023
Merged

Conversation

Naghasan
Copy link
Contributor

@Naghasan Naghasan commented Aug 29, 2023

The patch add support for HIP via the libamd_comgr:

  • Add build path in sycl-fusion
  • Add finalization routine inside hip adapter's build program

The patch add support for HIP via the libamd_comgr:

- Add build path in sycl-fusion
- Add finalization routine inside hip adapter's build program

Signed-off-by: Victor Lomuller <victor@codeplay.com>
@Naghasan Naghasan force-pushed the victor/kernel-fusion-amd branch from 3e27075 to 803ca51 Compare September 18, 2023 12:22
@Naghasan Naghasan marked this pull request as ready for review September 18, 2023 12:24
@Naghasan Naghasan requested review from victor-eds, sommerlukas and a team as code owners September 18, 2023 12:24
Copy link
Contributor

@sommerlukas sommerlukas left a comment

Choose a reason for hiding this comment

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

Mostly looks good, just a few nits and questions.

Copy link
Contributor

@sommerlukas sommerlukas left a comment

Choose a reason for hiding this comment

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

A few other nits outside of code changes:

@Naghasan Naghasan changed the title [KernelFusion] Add HIP support [SYCL][Fusion] Add HIP support Sep 19, 2023
Copy link
Contributor

@sommerlukas sommerlukas left a comment

Choose a reason for hiding this comment

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

LGTM, just a few minor nits in documentation text.

@sommerlukas sommerlukas self-requested a review September 20, 2023 07:39
Co-authored-by: Lukas Sommer <lukas.sommer@codeplay.com>
Copy link
Contributor

@sommerlukas sommerlukas left a comment

Choose a reason for hiding this comment

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

Sorry, only reviewed one of the commits previously.

Found a few more nits, but overall looks good.

@Naghasan Naghasan temporarily deployed to WindowsCILock September 26, 2023 22:12 — with GitHub Actions Inactive
@Naghasan Naghasan temporarily deployed to WindowsCILock October 2, 2023 13:37 — with GitHub Actions Inactive
@Naghasan Naghasan temporarily deployed to WindowsCILock October 2, 2023 14:13 — with GitHub Actions Inactive
@Naghasan Naghasan requested a review from a team as a code owner October 9, 2023 20:08
@Naghasan Naghasan temporarily deployed to WindowsCILock October 9, 2023 20:09 — with GitHub Actions Inactive
@Naghasan Naghasan temporarily deployed to WindowsCILock October 9, 2023 20:48 — with GitHub Actions Inactive
@Naghasan
Copy link
Contributor Author

ping @intel/dpcpp-l0-pi-reviewers @intel/llvm-reviewers-runtime

@kbenzie
Copy link
Contributor

kbenzie commented Oct 25, 2023

@Naghasan now that oneapi-src/unified-runtime#940 is merged please pull in the latest sycl branch changes then update the UNIFIED_RUNTIME_TAG as follows:

diff --git a/sycl/plugins/unified_runtime/CMakeLists.txt b/sycl/plugins/unified_runtime/CMakeLists.txt
index 20aae8927f0b..88b328e8daab 100644
--- a/sycl/plugins/unified_runtime/CMakeLists.txt
+++ b/sycl/plugins/unified_runtime/CMakeLists.txt
@@ -54,13 +54,13 @@ if(SYCL_PI_UR_USE_FETCH_CONTENT)
   include(FetchContent)

   set(UNIFIED_RUNTIME_REPO "https://github.com/oneapi-src/unified-runtime.git")
-  # commit 3a3aae387eaa8aa072ff310b94df83c9d1a33a0b
-  # Merge: 614e6d0 b4425bc
+  # commit cf26de283a1233e6c93feb085acc10c566888b59
+  # Merge: 3a3aae38 2fd9dea2
   # Author: Kenneth Benzie (Benie) <k.benzie@codeplay.com>
-  # Date:   Mon Oct 23 14:49:01 2023 +0100
-  #     Merge pull request #945 from npmiller/fix-priority
-  #     [CUDA] Fix queue creation with native handle
-  set(UNIFIED_RUNTIME_TAG 3a3aae387eaa8aa072ff310b94df83c9d1a33a0b)
+  # Date:   Wed Oct 25 10:36:48 2023 +0100
+  #     Merge pull request #940 from Naghasan/victor/kernel-fusion-amd
+  #     [UR][HIP] Enable kernel finalization using comgr
+  set(UNIFIED_RUNTIME_TAG cf26de283a1233e6c93feb085acc10c566888b59)

   if(SYCL_PI_UR_OVERRIDE_FETCH_CONTENT_REPO)
     set(UNIFIED_RUNTIME_REPO "${SYCL_PI_UR_OVERRIDE_FETCH_CONTENT_REPO}")

@Naghasan
Copy link
Contributor Author

thanks @kbenzie

@Naghasan Naghasan temporarily deployed to WindowsCILock October 25, 2023 14:30 — with GitHub Actions Inactive
@Naghasan
Copy link
Contributor Author

ping @intel/llvm-reviewers-runtime

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Runtime changes LGTM!

@Naghasan Naghasan temporarily deployed to WindowsCILock October 25, 2023 15:06 — with GitHub Actions Inactive
@Naghasan Naghasan temporarily deployed to WindowsCILock October 25, 2023 15:36 — with GitHub Actions Inactive
@Naghasan Naghasan temporarily deployed to WindowsCILock October 25, 2023 16:12 — with GitHub Actions Inactive
@againull againull merged commit 06f7bbf into intel:sycl Oct 25, 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.

10 participants