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][HIP] Implement mem_advise for HIP #1027

Merged
merged 7 commits into from
Jan 9, 2024

Conversation

GeorgeWeb
Copy link
Contributor

Adds initial implementation for mem_advise to the HIP adapter.

@GeorgeWeb GeorgeWeb requested a review from a team as a code owner November 1, 2023 17:44
@GeorgeWeb
Copy link
Contributor Author

Related intel/llvm PR: intel/llvm#10697

@JackAKirk
Copy link
Contributor

I just remembered that this is not consistent with the change to the spec made here: https://github.com/oneapi-src/unified-runtime/pull/854/files

See how the cuda behaviour has been changed here: #989

I think the behaviour should at least be consistent across adapters.

@GeorgeWeb
Copy link
Contributor Author

I just remembered that this is not consistent with the change to the spec made here: https://github.com/oneapi-src/unified-runtime/pull/854/files

See how the cuda behaviour has been changed here: #989

I think the behaviour should at least be consistent across adapters.

@JackAKirk Good catch. This PR was moved from intel/llvm where it was open for a while so I kinda missed the UR-spec related changes. Will update, thanks for spotting this.

@JackAKirk
Copy link
Contributor

I just remembered that this is not consistent with the change to the spec made here: https://github.com/oneapi-src/unified-runtime/pull/854/files
See how the cuda behaviour has been changed here: #989
I think the behaviour should at least be consistent across adapters.

@JackAKirk Good catch. This PR was moved from intel/llvm where it was open for a while so I kinda missed the UR-spec related changes. Will update, thanks for spotting this.

no worries, otherwise LGTM.

source/adapters/hip/enqueue.cpp Outdated Show resolved Hide resolved
source/adapters/hip/enqueue.cpp Show resolved Hide resolved
@GeorgeWeb GeorgeWeb added the ready to merge Added to PR's which are ready to merge label Nov 14, 2023
@fabiomestre fabiomestre changed the base branch from adapters to main December 5, 2023 16:51
@fabiomestre
Copy link
Contributor

I have updated the target branch of this PR from the adapters branch to the main branch.
Development in UR is moving back to main. The adapters branch will soon be deleted.

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e69ed21) 15.70% compared to head (62d0934) 15.70%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1027   +/-   ##
=======================================
  Coverage   15.70%   15.70%           
=======================================
  Files         223      223           
  Lines       31518    31518           
  Branches     3556     3556           
=======================================
  Hits         4951     4951           
  Misses      26516    26516           
  Partials       51       51           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kbenzie
Copy link
Contributor

kbenzie commented Jan 8, 2024

@GeorgeWeb FYI I rebased this on top of main to clean up the history aiming to merge this next.

@kbenzie kbenzie merged commit 12a67f5 into oneapi-src:main Jan 9, 2024
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Added to PR's which are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants