[UR] Remove UMF sources and use standalone UMF repo instead #1216

igchor · 2024-01-02T23:57:09Z

No description provided.

ldrumm · 2024-01-12T16:33:19Z

Do you have a rationale for such a change? Splitting repos can make synchronization quite hard despite some marginal maintenance-adjacent benefits. I'm wary about such splits while it still requires 3 merge requests between UR and intel/llvm before a patch is truly landed. Until that situation is sorted, I fear a change like this will compound this maintenance complexity.

Thoughts?

bratpiorka · 2024-01-12T16:48:32Z

Do you have a rationale for such a change? Splitting repos can make synchronization quite hard despite some marginal maintenance-adjacent benefits. I'm wary about such splits while it still requires 3 merge requests between UR and intel/llvm before a patch is truly landed. Until that situation is sorted, I fear a change like this will compound this maintenance complexity.

Thoughts?

UMF has become a separate project with its own repository. It is currently under active development and is intended to be used by various oneAPI components, not just UR. All further enhancements and fixes will only go into the UMF repository - we do not plan to merge such changes here in the UR repository and into the UMF repository separately, so downloading UMF from its own repository makes it easier to manage.

igchor · 2024-01-16T22:59:29Z

@kbenzie could you please take a look?

Also, I have one question: Would it be OK if UMF (and hence UR) had a dependency on hwloc?

Hwloc will be needed to enable 'OS memory provider' that is currently disabled and for other features like topology discovery that will be used by other oneAPI components.

kbenzie · 2024-01-17T16:18:04Z

@kbenzie could you please take a look?

As for UMF being a separate project, I think this change makes sense. 👍

Also, I have one question: Would it be OK if UMF (and hence UR) had a dependency on hwloc?

Hwloc will be needed to enable 'OS memory provider' that is currently disabled and for other features like topology discovery that will be used by other oneAPI components.

Would the dependency be dynamic? In principal I don't have an issue with the dependency but I'd like to avoid a situation where a user doesn't have hwloc available and that causes SYCL programs to crash.

ldrumm · 2024-01-17T16:34:14Z

UMF has become a separate project with its own repository. It is currently under active development and is intended to be used by various oneAPI components, not just UR. All further enhancements and fixes will only go into the UMF repository - we do not plan to merge such changes here in the UR repository and into the UMF repository separately, so downloading UMF from its own repository makes it easier to manage.

Thanks. That should go in the body of the commit message. Otherwise, I defer to kbenzie's expert judgment

igchor · 2024-01-17T21:26:27Z

UMF has become a separate project with its own repository. It is currently under active development and is intended to be used by various oneAPI components, not just UR. All further enhancements and fixes will only go into the UMF repository - we do not plan to merge such changes here in the UR repository and into the UMF repository separately, so downloading UMF from its own repository makes it easier to manage.

Thanks. That should go in the body of the commit message. Otherwise, I defer to kbenzie's expert judgment

Of course, done.

igchor · 2024-01-17T21:28:46Z

@kbenzie could you please take a look?

As for UMF being a separate project, I think this change makes sense. 👍

Also, I have one question: Would it be OK if UMF (and hence UR) had a dependency on hwloc?
Hwloc will be needed to enable 'OS memory provider' that is currently disabled and for other features like topology discovery that will be used by other oneAPI components.

Would the dependency be dynamic? In principal I don't have an issue with the dependency but I'd like to avoid a situation where a user doesn't have hwloc available and that causes SYCL programs to crash.

Ideally hwloc would be a static dependency. As far as I know hwloc is already distributed inside oneapi packages so there should not be any issues with distribution. (But in any way, I do not add any extra dependency in this PR).

codecov-commenter · 2024-01-17T21:39:17Z

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (b51dec0) 15.58% compared to head (5fd1722) 13.21%.
Report is 5 commits behind head on main.

Files	Patch %	Lines
...e/common/umf_pools/disjoint_pool_config_parser.cpp	0.00%	15 Missing ⚠️
source/common/umf_helpers.hpp	0.00%	9 Missing ⚠️
test/usm/usmPoolManager.cpp	0.00%	5 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1216      +/-   ##
==========================================
- Coverage   15.58%   13.21%   -2.38%     
==========================================
  Files         232      220      -12     
  Lines       32089    31194     -895     
  Branches     3638     3551      -87     
==========================================
- Hits         5001     4121     -880     
- Misses      27037    27069      +32     
+ Partials       51        4      -47

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

kbenzie · 2024-01-18T16:17:26Z

Ideally hwloc would be a static dependency. As far as I know hwloc is already distributed inside oneapi packages so there should not be any issues with distribution. (But in any way, I do not add any extra dependency in this PR).

Oh, if its statically linked users shouldn't even notice, right? In that case I don't see any issues adding the dependency.

igchor · 2024-01-18T18:06:58Z

Ideally hwloc would be a static dependency. As far as I know hwloc is already distributed inside oneapi packages so there should not be any issues with distribution. (But in any way, I do not add any extra dependency in this PR).

Oh, if its statically linked users shouldn't even notice, right? In that case I don't see any issues adding the dependency.

Actually, sorry, I'm not yet sure if we'll link statically or dynamically. I will take a look at how other oneAPI components are using it and I'll specify that when I create a PR with adding the dependency. We can discuss it then and change the approach if necessary.

igchor · 2024-01-22T18:22:24Z

@kbenzie are you OK merging this PR as is, or do you need more time to review it?

kbenzie · 2024-01-23T12:11:54Z

@kbenzie are you OK merging this PR as is, or do you need more time to review it?

We'll need approvals from all the touched 4 adapters (@oneapi-src/unified-runtime-opencl-write, @oneapi-src/unified-runtime-hip-write, @oneapi-src/unified-runtime-cuda-write, @oneapi-src/unified-runtime-level-zero-write) as well as shared code (@oneapi-src/unified-runtime-maintain).

I'll take a look for the OpenCL adapter and shared code but the other adapter teams will need to approve also.

igchor · 2024-01-23T15:41:14Z

@ldrumm could you please take a look at HIP and CUDA changes?
@nrspruit could you please take a look at L0 changes?

Changes in the adapters are pretty minor - I only change the way the pool is being created (so that we now use UMF_DISJOINT_POOL_OPS from UMF repo)

nrspruit

LGTM for level-zero

ldrumm · 2024-01-23T17:17:39Z

source/common/CMakeLists.txt

@@ -19,9 +17,31 @@ target_include_directories(ur_common PUBLIC
    ${CMAKE_SOURCE_DIR}/include
 )

+message(STATUS "Download Unified Memory Framework from github.com")


In dpc++ we have a switch to enable building against a local checkout of unified runtime. Should we have the same thing here?

c.f intel/llvm/139b13172

I don't think there will be many changes required in the UR when bumping UMF version in the future.

However, we've had some discussion about potentially moving some of the memory provider logic for L0 and CUDA to UMF - if we decide to do that, then adding such a switch might a good idea.

ldrumm

HIP and CUDA look fine

lukaszstolarczuk

👍

igchor · 2024-02-01T17:18:15Z

FYI, I've create a testing PR to llvm (with this patch rebased on main from a few days ago): intel/llvm#12570

igchor · 2024-02-07T18:41:01Z

@kbenzie do you have an estimate on when you will merge it? is there any extra testing you would like me to do?

kbenzie · 2024-02-12T16:08:17Z

@kbenzie do you have an estimate on when you will merge it? is there any extra testing you would like me to do?

I was on holiday last week. No additionally testing necessary, we'll aim to merge it this week.

kbenzie · 2024-02-13T17:15:14Z

@igchor I rebased this to test against the latest main changes before merging, seems like a fix in required for the native cpu cmake, hopefull its a straight forward fix?

CMake Error at cmake/helpers.cmake:131 (add_library):
  Target "ur_adapter_native_cpu" links to target
  "unified-runtime::unified_malloc_framework" but the target was not found.
  Perhaps a find_package() call is missing for an IMPORTED target, or an
  ALIAS target is missing?
Call Stack (most recent call first):
  source/adapters/CMakeLists.txt:7 (add_ur_library)
  source/adapters/native_cpu/CMakeLists.txt:10 (add_ur_adapter)

UMF has become a separate project with its own repository. It is currently under active development and is intended to be used by various oneAPI components, not just UR. All further enhancements and fixes will only go into the UMF repository - we do not plan to merge such changes here in the UR repository and into the UMF repository separately.

igchor · 2024-02-13T20:26:34Z

@igchor I rebased this to test against the latest main changes before merging, seems like a fix in required for the native cpu cmake, hopefull its a straight forward fix?

CMake Error at cmake/helpers.cmake:131 (add_library):
  Target "ur_adapter_native_cpu" links to target
  "unified-runtime::unified_malloc_framework" but the target was not found.
  Perhaps a find_package() call is missing for an IMPORTED target, or an
  ALIAS target is missing?
Call Stack (most recent call first):
  source/adapters/CMakeLists.txt:7 (add_ur_library)
  source/adapters/native_cpu/CMakeLists.txt:10 (add_ur_adapter)

Yes, I just had to remove reference to unified-runtime::unified_malloc_framework from native CPU CMakeLists.txt - it's enough to just link with ur_common now.

oneapi-src/unified-runtime#1216 --------- Co-authored-by: Kenneth Benzie (Benie) <k.benzie@codeplay.com>

oneapi-src#1216 mistakenly removed linking adapters with Threads library (threads lib was linked with disjoint_pool). Fix this.

Before oneapi-src#1216 adapters linked with disjoint_pool library that linked with Threads library. PR oneapi-src#1216 replace disjoint_pool with a version from UMF library that does not link with Threads lib explicitly which leads to compilation error on xmain. Fix this by explicitly adding adding Threads to ur_common.

igchor force-pushed the umf_standalone branch 2 times, most recently from 06b1194 to 903172f Compare January 10, 2024 19:28

igchor marked this pull request as ready for review January 10, 2024 20:33

igchor requested review from a team as code owners January 10, 2024 20:33

igchor requested a review from mmoadeli January 10, 2024 20:33

bratpiorka approved these changes Jan 11, 2024

View reviewed changes

ldorau approved these changes Jan 12, 2024

View reviewed changes

ldrumm requested a review from kbenzie January 12, 2024 16:33

igchor force-pushed the umf_standalone branch from 903172f to 7fa65fe Compare January 17, 2024 21:26

igchor force-pushed the umf_standalone branch from 7fa65fe to 2b907c4 Compare January 17, 2024 21:32

igchor force-pushed the umf_standalone branch from 2b907c4 to 5fd1722 Compare January 18, 2024 00:18

kbenzie approved these changes Jan 23, 2024

View reviewed changes

nrspruit approved these changes Jan 23, 2024

View reviewed changes

ldrumm reviewed Jan 23, 2024

View reviewed changes

ldrumm approved these changes Jan 23, 2024

View reviewed changes

kbenzie added the ready to merge Added to PR's which are ready to merge label Jan 24, 2024

lukaszstolarczuk reviewed Jan 24, 2024

View reviewed changes

kbenzie force-pushed the umf_standalone branch from 5fd1722 to e3eff58 Compare February 13, 2024 16:45

igchor force-pushed the umf_standalone branch from e3eff58 to db5c33b Compare February 13, 2024 20:25

igchor requested a review from a team as a code owner February 13, 2024 20:25

kbenzie merged commit cfba9f1 into oneapi-src:main Feb 14, 2024
52 checks passed

kbenzie mentioned this pull request Feb 14, 2024

[UR] Remove UMF sources and use standalone UMF repo instead intel/llvm#12570

Merged

dm-vodopyanov pushed a commit to intel/llvm that referenced this pull request Feb 14, 2024

[UR] Remove UMF sources and use standalone UMF repo instead (#12570)

571834c

oneapi-src/unified-runtime#1216 --------- Co-authored-by: Kenneth Benzie (Benie) <k.benzie@codeplay.com>

igchor deleted the umf_standalone branch February 14, 2024 17:08

igchor added a commit to igchor/unified-runtime that referenced this pull request Feb 14, 2024

[UR] Add missing Threads library to common

2fd7ac3

oneapi-src#1216 mistakenly removed linking adapters with Threads library (threads lib was linked with disjoint_pool). Fix this.

igchor mentioned this pull request Feb 14, 2024

[UR] Add missing Threads library to common #1343

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UR] Remove UMF sources and use standalone UMF repo instead #1216

[UR] Remove UMF sources and use standalone UMF repo instead #1216

igchor commented Jan 2, 2024

ldrumm commented Jan 12, 2024

bratpiorka commented Jan 12, 2024

igchor commented Jan 16, 2024 •

edited

Loading

kbenzie commented Jan 17, 2024 •

edited

Loading

ldrumm commented Jan 17, 2024

igchor commented Jan 17, 2024

igchor commented Jan 17, 2024 •

edited

Loading

codecov-commenter commented Jan 17, 2024 •

edited

Loading

kbenzie commented Jan 18, 2024

igchor commented Jan 18, 2024

igchor commented Jan 22, 2024

kbenzie commented Jan 23, 2024

igchor commented Jan 23, 2024

nrspruit left a comment

ldrumm Jan 23, 2024 •

edited

Loading

igchor Jan 23, 2024

ldrumm left a comment

lukaszstolarczuk left a comment

igchor commented Feb 1, 2024

igchor commented Feb 7, 2024

kbenzie commented Feb 12, 2024

kbenzie commented Feb 13, 2024

igchor commented Feb 13, 2024

[UR] Remove UMF sources and use standalone UMF repo instead #1216

[UR] Remove UMF sources and use standalone UMF repo instead #1216

Conversation

igchor commented Jan 2, 2024

ldrumm commented Jan 12, 2024

bratpiorka commented Jan 12, 2024

igchor commented Jan 16, 2024 • edited Loading

kbenzie commented Jan 17, 2024 • edited Loading

ldrumm commented Jan 17, 2024

igchor commented Jan 17, 2024

igchor commented Jan 17, 2024 • edited Loading

codecov-commenter commented Jan 17, 2024 • edited Loading

Codecov Report

kbenzie commented Jan 18, 2024

igchor commented Jan 18, 2024

igchor commented Jan 22, 2024

kbenzie commented Jan 23, 2024

igchor commented Jan 23, 2024

nrspruit left a comment

Choose a reason for hiding this comment

ldrumm Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

igchor Jan 23, 2024

Choose a reason for hiding this comment

ldrumm left a comment

Choose a reason for hiding this comment

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

igchor commented Feb 1, 2024

igchor commented Feb 7, 2024

kbenzie commented Feb 12, 2024

kbenzie commented Feb 13, 2024

igchor commented Feb 13, 2024

igchor commented Jan 16, 2024 •

edited

Loading

kbenzie commented Jan 17, 2024 •

edited

Loading

igchor commented Jan 17, 2024 •

edited

Loading

codecov-commenter commented Jan 17, 2024 •

edited

Loading

ldrumm Jan 23, 2024 •

edited

Loading