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

Add usm memory constructor from existing allocation #1782

Merged

Conversation

oleksandr-pavlyk
Copy link
Collaborator

This PR add dpctl::memory::usm_memory constructor to create Python memory object from natively made allocation.

The signature is

usm_memory(void *usm_ptr, size_t nbytes, sycl::queue &q, std::shared_ptr<void> owner)

where usm_ptr is the pointer to USM-allocation, nbytes the size of allocation in bytes, q the queue associated with the Python memory object, and owner is the shared pointer whose destructor frees USM allocation via its custom deleter argument.

Example of using such constructor is added to examples/pybind11/external_usm_allocation extension.

  • Have you provided a meaningful PR description?
  • Have you added a test, reproducer or referred to an issue with a reproducer?
  • Have you tested your changes locally for CPU and GPU devices?
  • Have you made sure that new changes do not introduce compiler warnings?
  • Have you checked performance impact of proposed changes?
  • If this PR is a work in progress, are you opening the PR as a draft?

Copy link

github-actions bot commented Aug 5, 2024

Deleted rendered PR docs from intelpython.github.com/dpctl, latest should be updated shortly. 🤞

@coveralls
Copy link
Collaborator

coveralls commented Aug 5, 2024

Coverage Status

coverage: 87.93%. remained the same
when pulling 65230a4 on add-usm-memory-constructor-from-existing-allocation
into b9242f4 on master.

Copy link

github-actions bot commented Aug 5, 2024

Array API standard conformance tests for dpctl=0.18.0dev0=py310ha798474_238 ran successfully.
Passed: 895
Failed: 0
Skipped: 119

Copy link

@AlexanderKalistratov AlexanderKalistratov left a comment

Choose a reason for hiding this comment

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

I have small style comments which can be ignored.

Copy link

github-actions bot commented Aug 5, 2024

Array API standard conformance tests for dpctl=0.18.0dev0=py310ha798474_239 ran successfully.
Passed: 894
Failed: 1
Skipped: 119

Copy link

github-actions bot commented Aug 5, 2024

Array API standard conformance tests for dpctl=0.18.0dev0=py310ha798474_240 ran successfully.
Passed: 894
Failed: 1
Skipped: 119

Copy link

github-actions bot commented Aug 6, 2024

Array API standard conformance tests for dpctl=0.18.0dev0=py310ha798474_254 ran successfully.
Passed: 894
Failed: 1
Skipped: 119

usm_memory(
    void *usm_ptr,
    size_t nbytes,
    sycl::queue &q,
    std::shared_ptr<void> shptr)

usm_ptr: Pointer to USM allocation

nbytes:  is the size of allocation in bytes

q : sycl::queue associated with this allocation in Python.

shptr: Smart pointer with custom deleter that deallocates
       the USM allocation.

Implementation notes:
Use unique_ptr to manage lifetime of new copy of sycl::queue,

Delegate newly created Python object memory management
to a unique_ptr to ensure it gets properly handled in case
an exception is thrown.
The example now also demonstration zero-copy creation of
MemoryUSMDevice Python object from allocation created
and populated in C++.

Use lambda as a deleter per @AlexanderKalistratov suggestion
@oleksandr-pavlyk oleksandr-pavlyk force-pushed the add-usm-memory-constructor-from-existing-allocation branch from 0b4032a to 1ef63da Compare August 6, 2024 20:49
@oleksandr-pavlyk
Copy link
Collaborator Author

@AlexanderKalistratov @ndgrigorian I have applied suggested changes, rebased and squashed commits. This is ready for next round of reviewing.

Copy link

github-actions bot commented Aug 6, 2024

Array API standard conformance tests for dpctl=0.18.0dev0=py310ha798474_261 ran successfully.
Passed: 894
Failed: 1
Skipped: 119

Copy link

github-actions bot commented Aug 6, 2024

Array API standard conformance tests for dpctl=0.18.0dev0=py310ha798474_257 ran successfully.
Passed: 894
Failed: 1
Skipped: 119

Copy link

github-actions bot commented Aug 6, 2024

Array API standard conformance tests for dpctl=0.18.0dev0=py310ha798474_258 ran successfully.
Passed: 894
Failed: 1
Skipped: 119

Copy link

@AlexanderKalistratov AlexanderKalistratov left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@ndgrigorian ndgrigorian left a comment

Choose a reason for hiding this comment

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

Looks good to me, I ran the example on CUDA as well.

@oleksandr-pavlyk oleksandr-pavlyk merged commit ff0d4ea into master Aug 8, 2024
48 of 49 checks passed
@oleksandr-pavlyk oleksandr-pavlyk deleted the add-usm-memory-constructor-from-existing-allocation branch August 8, 2024 09:55
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.

4 participants