[HIP] Hip adapter multi dev ctx #999

hdelan · 2023-10-26T10:39:33Z

Moving intel/llvm#11105 into UR repo with a few changes. Depends on #961

The getter func for ptrs/arrays/surfaces now allocates lazily if needed. This has made code a lot cleaner.

kbenzie · 2023-10-26T10:51:08Z

source/ur/ur.hpp

We'll need to sync changes to this file with the changes in #997

jchlanda · 2023-10-26T10:53:52Z

Are you sure this is only multi device context, some of the changes look familiar, maybe the std::variant instead of union got mixed in?

kbenzie · 2023-10-26T10:54:42Z

Are you sure this is only multi device context, some of the changes look familiar, maybe the std::variant instead of union got mixed in?

If this is dependant on the std::variant changes, this should be marked draft until those merge.

hdelan · 2023-10-26T10:55:41Z

Are you sure this is only multi device context, some of the changes look familiar, maybe the std::variant instead of union got mixed in?

This depends on the std::variant stuff so I have included the commit. Once that gets merged then I will rebase and the diff will be smaller. You could maybe wait on reviewing this until that merges

jchlanda · 2023-10-26T10:57:08Z

Are you sure this is only multi device context, some of the changes look familiar, maybe the std::variant instead of union got mixed in?

This depends on the std::variant stuff so I have included the commit. Once that gets merged then I will rebase and the diff will be smaller. You could maybe wait on reviewing this until that merges

Sure, will do. Consider marking PR as draft if they are not ready for review.

hdelan · 2023-10-27T14:25:06Z

test/conformance/context/context_adapter_hip.match

@@ -1 +1,2 @@
 urContextCreateWithNativeHandleTest.Success/AMD_HIP_BACKEND___{{.*}}_
+urContextGetInfoTestWithInfoParam.Success/AMD_HIP_BACKEND___{{.*}}


The size of the Devices returned has changed as we now have a vector of devices. Whereas before we just had a pointer. This is backend specific so I think maybe just keep this as failing for now

hdelan · 2023-10-27T14:25:50Z

test/conformance/memory/memory_adapter_hip.match

+urMemGetInfoTest.InvalidNullPointerPropSizeRet/AMD_HIP_BACKEND___{{.*}}
+urMemGetInfoTest.InvalidNullPointerPropSizeRet/AMD_HIP_BACKEND___{{.*}}
+urMemImageCreateTest.InvalidSize/AMD_HIP_BACKEND___{{.*}}
+urMemImageGetInfoTest.Success/AMD_HIP_BACKEND___{{.*}}


These MemImageGetInfos are unsupported

Sounds like this test needs to allow these being unsupported. I can look into that.

OK great. Will I change this .match or just add a TODO?

I expect it'll be a separate PR so this line will need to stay in the .match file in order to pass testing. I don't actually know if these .match files support comments.

#1144 is the PR that will address this CTS issue. Can resolve this discussion, I think.

hdelan · 2023-10-27T14:26:36Z

test/conformance/memory/memory_adapter_hip.match

+urMemGetInfoTest.InvalidNullPointerParamValue/AMD_HIP_BACKEND___{{.*}}
+urMemGetInfoTest.InvalidNullPointerPropSizeRet/AMD_HIP_BACKEND___{{.*}}
+urMemGetInfoTest.InvalidNullPointerPropSizeRet/AMD_HIP_BACKEND___{{.*}}
+urMemImageCreateTest.InvalidSize/AMD_HIP_BACKEND___{{.*}}


I'm not sure how to deal with this thing. Perhaps we need to query the max image width depth etc when a device is initialized so we can check vals against this at Image creation

hdelan · 2023-10-27T14:28:38Z

test/conformance/memory/memory_adapter_hip.match

-urMemBufferCreateWithNativeHandleTest.Success/AMD_HIP_BACKEND___{{.*}}_
-Segmentation fault
+urMemGetInfoTest.Success/AMD_HIP_BACKEND___{{.*}}
+urMemGetInfoTest.InvalidNullPointerParamValue/AMD_HIP_BACKEND___{{.*}}


These are returning early in the adapter, see here https://github.com/oneapi-src/unified-runtime/blob/adapters/source/loader/layers/validation/ur_valddi.cpp#L1350 so the adapter entry point is never called

source/adapters/cuda/memory.hpp

jchlanda

That's quite a heavy lifting you've done here, well done!

source/adapters/hip/context.hpp

source/adapters/hip/memory.cpp

source/adapters/hip/memory.hpp

kbenzie · 2023-11-24T17:31:26Z

The merge conflicts will need to be resolve.

hdelan · 2023-11-24T18:21:19Z

@kbenzie have rebased

hdelan · 2023-11-24T18:47:20Z

intel/llvm#11739 DPC++ PR here

codecov-commenter · 2023-12-01T17:37:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fe2735a) 15.79% compared to head (f0e0be2) 15.79%.

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

Additional details and impacted files

@@            Coverage Diff            @@
##           adapters     #999   +/-   ##
=========================================
  Coverage     15.79%   15.79%           
=========================================
  Files           223      223           
  Lines         31351    31351           
  Branches       3511     3511           
=========================================
  Hits           4951     4951           
  Misses        26349    26349           
  Partials         51       51

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

kbenzie

Couple of warnings treated as errors causing the GHA failure.

source/adapters/hip/usm.cpp

hdelan · 2023-12-04T14:03:08Z

Oops thanks for that @kbenzie

Update SHA for oneapi-src/unified-runtime#999 UR hip adapter multi device context --------- Co-authored-by: Kenneth Benzie (Benie) <[email protected]>

hdelan requested review from a team as code owners October 26, 2023 10:39

hdelan requested review from jchlanda and npmiller October 26, 2023 10:43

kbenzie reviewed Oct 26, 2023

View reviewed changes

source/ur/ur.hpp Outdated

Copy link

Contributor

kbenzie Oct 26, 2023

Choose a reason for hiding this comment

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

We'll need to sync changes to this file with the changes in #997

hdelan force-pushed the hip-adapter-multi-dev-ctx branch from 864294c to ebdb923 Compare October 26, 2023 10:54

hdelan marked this pull request as draft October 26, 2023 10:55

hdelan force-pushed the hip-adapter-multi-dev-ctx branch from ebdb923 to 263d84b Compare October 27, 2023 14:23

hdelan commented Oct 27, 2023

View reviewed changes

hdelan force-pushed the hip-adapter-multi-dev-ctx branch from 263d84b to 32419b6 Compare October 27, 2023 14:30

veselypeta reviewed Oct 27, 2023

View reviewed changes

source/adapters/cuda/memory.hpp Outdated Show resolved Hide resolved

source/adapters/cuda/memory.hpp Outdated Show resolved Hide resolved

hdelan force-pushed the hip-adapter-multi-dev-ctx branch 2 times, most recently from cf65a5d to 3743e6c Compare November 1, 2023 09:55

hdelan mentioned this pull request Nov 1, 2023

[UR] Run tests for hip multi dev ctx intel/llvm#11739

Merged

hdelan force-pushed the hip-adapter-multi-dev-ctx branch 6 times, most recently from b4bd0f4 to b811c86 Compare November 2, 2023 15:16

hdelan marked this pull request as ready for review November 2, 2023 15:20

hdelan force-pushed the hip-adapter-multi-dev-ctx branch from b811c86 to 8de5817 Compare November 2, 2023 15:46

jchlanda reviewed Nov 8, 2023

View reviewed changes

hdelan force-pushed the hip-adapter-multi-dev-ctx branch from 285e286 to 7e975e9 Compare November 10, 2023 10:11

jchlanda approved these changes Nov 10, 2023

View reviewed changes

hdelan added the ready to merge Added to PR's which are ready to merge label Nov 15, 2023

hdelan force-pushed the hip-adapter-multi-dev-ctx branch 2 times, most recently from 5ab1690 to 3934569 Compare November 24, 2023 18:13

hdelan closed this Nov 27, 2023

hdelan reopened this Nov 27, 2023

hdelan force-pushed the hip-adapter-multi-dev-ctx branch from 3934569 to 7118ac6 Compare December 1, 2023 16:18

kbenzie reviewed Dec 4, 2023

View reviewed changes

source/adapters/hip/usm.cpp Show resolved Hide resolved

source/adapters/hip/usm.cpp Show resolved Hide resolved

hdelan and others added 2 commits December 4, 2023 14:49

HIP adapter multi dev ctx

5dc6e46

[HIP] Fix unused parameter warnings

f0e0be2

kbenzie force-pushed the hip-adapter-multi-dev-ctx branch from a2b5a00 to f0e0be2 Compare December 4, 2023 14:50

kbenzie merged commit 67e7da3 into oneapi-src:adapters Dec 5, 2023

This was referenced Dec 5, 2023

gpu: nvidia: amd: Get native context through device oneapi-src/oneDNN#1765

Merged

Native context reinterpret casted as pi_context uxlfoundation/oneMath#424

Closed

[CUDA][HIP] Use device to get native context uxlfoundation/oneMath#425

Merged

hdelan mentioned this pull request Dec 18, 2023

[draft][HIP][CUDA] Use new UR entry point to get native mem with device param intel/llvm#12199

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HIP] Hip adapter multi dev ctx #999

[HIP] Hip adapter multi dev ctx #999

hdelan commented Oct 26, 2023

kbenzie Oct 26, 2023

jchlanda commented Oct 26, 2023

kbenzie commented Oct 26, 2023 •

edited

Loading

hdelan commented Oct 26, 2023

jchlanda commented Oct 26, 2023

hdelan Oct 27, 2023 •

edited

Loading

hdelan Oct 27, 2023

kbenzie Nov 30, 2023

hdelan Nov 30, 2023

kbenzie Dec 1, 2023

kbenzie Dec 1, 2023

hdelan Oct 27, 2023

hdelan Oct 27, 2023

jchlanda left a comment

kbenzie commented Nov 24, 2023

hdelan commented Nov 24, 2023

hdelan commented Nov 24, 2023

codecov-commenter commented Dec 1, 2023 •

edited

Loading

kbenzie left a comment

hdelan commented Dec 4, 2023

		@@ -1 +1,2 @@
		urContextCreateWithNativeHandleTest.Success/AMD_HIP_BACKEND___{{.*}}_
		urContextGetInfoTestWithInfoParam.Success/AMD_HIP_BACKEND___{{.*}}

[HIP] Hip adapter multi dev ctx #999

[HIP] Hip adapter multi dev ctx #999

Conversation

hdelan commented Oct 26, 2023

Choose a reason for hiding this comment

jchlanda commented Oct 26, 2023

kbenzie commented Oct 26, 2023 • edited Loading

hdelan commented Oct 26, 2023

jchlanda commented Oct 26, 2023

hdelan Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchlanda left a comment

Choose a reason for hiding this comment

kbenzie commented Nov 24, 2023

hdelan commented Nov 24, 2023

hdelan commented Nov 24, 2023

codecov-commenter commented Dec 1, 2023 • edited Loading

Codecov Report

kbenzie left a comment

Choose a reason for hiding this comment

hdelan commented Dec 4, 2023

kbenzie commented Oct 26, 2023 •

edited

Loading

hdelan Oct 27, 2023 •

edited

Loading

codecov-commenter commented Dec 1, 2023 •

edited

Loading