Skip to content

Move Unified Memory Allocation sources to UR-common #182

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Feb 1, 2023
Merged

Move Unified Memory Allocation sources to UR-common #182

merged 3 commits into from
Feb 1, 2023

Conversation

igchor
Copy link
Member

@igchor igchor commented Jan 23, 2023 •
edited
Loading

Initially, I wanted to move this into llvm repo, but having this here will simplify development. Implementation of the L0 memory provider will be a part of L0 adapter (so inside llvm for now). The same is true for a heap manager implementation. Right now, the idea is to use usm_allocator (this requires following 2 PRs to be merged: intel/llvm#7853 intel/llvm#7785)

List of things to do after this initial PR is merged:

  • extend error codes
  • add GetLastResult, as in UR
  • consider creating a generic config structure for pools/providers
  • allow to build UMA as shared library for testing (e.g. with openMP)

@jandres742
Copy link

thanks @igchor . could you clarify why are we adding the USM memory allocator to this repo (where we have the loader and spec), instead of directly to LLVM? From comment above, it's not clear to me whether that is just temporary while the other PRs are merged.

Or this is just a shim layer, that will call each adapter's memory allocator, and that's where we will have all optimizations?

jandres742
jandres742 reviewed Jan 24, 2023
View reviewed changes
jandres742
jandres742 reviewed Jan 24, 2023
View reviewed changes
jandres742
jandres742 reviewed Jan 24, 2023
View reviewed changes
jandres742
jandres742 reviewed Jan 24, 2023
View reviewed changes
@pbalcer
Copy link
Contributor

pbalcer commented Jan 24, 2023

This repo is meant to contain what we call common, which are the shared bits between the loader and all the adapters (but still statically linked with each). This is to allow for adapter implementations to be independent of LLVM, located either in this repo or somewhere else entirely, with the only mandatory dependency being the UR itself.

For the time being, some of the common utilities live in the LLVM repo. Still, as time goes on, we want to successively move them here - so that they are genuinely 'common', shared among not just the adapters that have to live in the LLVM repo for now but also others that might be somewhere else.

pbalcer
pbalcer reviewed Jan 24, 2023
View reviewed changes
Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

Please add a simple test that at least touches every entry point.

source/common/unified_memory_allocation/include/uma/memory_pool_ops.h Outdated

// TODO: create macro for intializing this structure (including version fields)
struct uma_memory_pool_ops_t {
uint64_t version_major;
Copy link
Contributor

Choose a reason for hiding this comment

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

you could create a base struct with versions and use it both here and in the provider.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've replaced it with a single variable and a macro to encode major/minor.

@igchor igchor force-pushed the unified_memory_allocation branch from 0cae200 to d025f93 Compare January 24, 2023 21:37
@igchor
Copy link
Member Author

igchor commented Jan 24, 2023

Please add a simple test that at least touches every entry point.

Done. I've thought about creating some C++ wrappers for memory pool/provider creation, like this: igchor@da0c5cb but decided against that for now. Since the library is in C, it would be good to have some pure C tests and the null/trace provider/pool can even be moved from tests to the actual library in the future.

@igchor
Copy link
Member Author

igchor commented Jan 24, 2023 •
edited
Loading

thanks @igchor . could you clarify why are we adding the USM memory allocator to this repo (where we have the loader and spec), instead of directly to LLVM? From comment above, it's not clear to me whether that is just temporary while the other PRs are merged.

Or this is just a shim layer, that will call each adapter's memory allocator, and that's where we will have all optimizations?

@jandres742 At this moment, the PR only contains header files and the shim layer. The actual memory provider implementation will be provided by each adapter + we can have some generic ones (like OS provider) implemented here.

For L0 memory provider the plan would be to have it alongside the L0 adapter (currently in llvm). As for memory pool implementations, initially, I'd like to use the usm_allocator that is implemented inside llvm. I think it's OK to keep the usm_allocator there until we move adapters here.

@jandres742
Copy link

thanks @igchor . could you clarify why are we adding the USM memory allocator to this repo (where we have the loader and spec), instead of directly to LLVM? From comment above, it's not clear to me whether that is just temporary while the other PRs are merged.
Or this is just a shim layer, that will call each adapter's memory allocator, and that's where we will have all optimizations?

@jandres742 At this moment, the PR only contains header files and the shim layer. The actual memory provider implementation will be provided by each adapter + we can have some generic ones (like OS provider) implemented here.

For L0 memory provider the plan would be to have it alongside the L0 adapter (currently in llvm). As for memory pool implementations, initially, I'd like to use the usm_allocator that is implemented inside llvm. I think it's OK to keep the usm_allocator there until we move adapters here.

Thanks @igchor . Yes, that is a good plan.

@igchor
Copy link
Member Author

igchor commented Jan 24, 2023

In my previous comment, I wrote that we could keep the usm_allocator (foundation for our generic memory pool implementation) inside llvm since the L0 adapter is in llvm. However, what about CUDA adapter?

@kbenzie @alycm are you already working on the CUDA adapter? Will the implementation be done here or in llvm repo? If it's here, then perhaps having the memory pool implementation in UR repo would actually be better,

pbalcer
pbalcer reviewed Jan 25, 2023
View reviewed changes
@igchor igchor force-pushed the unified_memory_allocation branch 2 times, most recently from 2ddeabf to f7c67bf Compare January 25, 2023 20:35
vinser52
vinser52 requested changes Jan 25, 2023
View reviewed changes
@igchor igchor force-pushed the unified_memory_allocation branch from f7c67bf to e3e4da3 Compare January 26, 2023 00:59
vinser52
vinser52 approved these changes Jan 26, 2023
View reviewed changes
vinser52
vinser52 reviewed Jan 27, 2023
View reviewed changes
@igchor igchor force-pushed the unified_memory_allocation branch 2 times, most recently from badf602 to 041dd14 Compare January 30, 2023 18:09
PatKamin
PatKamin reviewed Jan 31, 2023
View reviewed changes
igchor added 3 commits January 31, 2023 15:41
@igchor
Move UMA sources to UR-common
97cce4e 
UMA (Unified Memory Allocation) is built as a static library.
This commit doe not include any specific provider/pool implementation.
This will be supplied later.

The goal of UMA is to unify memory allocation between different
adapters. It is supposed to be a single point of optimizations for
memory operations (allocation, and in future, data movement).

UMA exposes pool and provider abstractions. Each adapter should
implement it's own memory provider for coarse-grain allocations.
On top of memory provider there will be memory pool that will offer
fine-grain allocations. Memory pool implementation can be shared
between all adapters or customized if necessary.
@igchor
Move fetching gtest to top-level tests CMakeLists
61325f8
@igchor
Add basic tests for UMA
424874e
@igchor igchor force-pushed the unified_memory_allocation branch 2 times, most recently from 5a93127 to 424874e Compare January 31, 2023 16:04
kbenzie
kbenzie approved these changes Feb 1, 2023
View reviewed changes
Copy link
Contributor

@kbenzie kbenzie left a comment

Choose a reason for hiding this comment

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

LGTM

@pbalcer pbalcer merged commit 36b133e into oneapi-src:main Feb 1, 2023
@igchor igchor deleted the unified_memory_allocation branch February 1, 2023 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vinser52 vinser52 vinser52 approved these changes

@pbalcer pbalcer pbalcer left review comments

@jandres742 jandres742 jandres742 left review comments

@PatKamin PatKamin PatKamin left review comments

@kbenzie kbenzie kbenzie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@igchor @jandres742 @pbalcer @kbenzie @vinser52 @PatKamin