Skip to content

[cleanup/tensor, step 3] Implement dpctl_capi loader in dpctl4pybind11 header file #932

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

Merged
merged 2 commits into from
Oct 15, 2022

Conversation

oleksandr-pavlyk
Copy link
Contributor

This PR creates dpctl::detail::dpctl_capi singleton class that load Cython-generated CAPI functions implemented in Cython modules of dpctl, and re-exports them as class members.

This PR removes another singleton dpctl::tensor::detail::usm_ndarray_types which is not a simple struct with trivial constructor.

This moves all initialization of static variables exported and implemented by Python into a single place, thus avoiding static order initialization fiasco.

  • 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?
  • If this PR is a work in progress, are you filing the PR as a draft?

The class implements a singleton pattern that populates global
variables exported by Cython CAPI functions via call to import_dpctl()
and then populates internal struct members with pointers populated
by Cython.

This loads both Cython 'public api' functions, types, and integer values.

dpctl::tensor::usm_ndarray and dpctl::memory::usm_memory classes, as
well as type-casters for dpctl.SyclQueue <-> sycl::queue,
dpctl.SyclDevice <-> sycl::device, etc. use dpctl_capi.

The singleton patterns helps avoid static initialization order fiasco
problem when using multiple translation units.

Single dpctl_capi loads typenum values, the
dpctl::tensor::detail::usm_ndarray_types struct no longer needs to be
a singleton, and becomes an ordinary struct with a static method.

tensor_py.cpp was changed to reflect that.
@oleksandr-pavlyk oleksandr-pavlyk changed the title Cleanup tensor step3 [cleanup/tensor, step 3] Implement dpctl_capi loader in dpctl4pybind11 header file Oct 13, 2022
@oleksandr-pavlyk oleksandr-pavlyk changed the base branch from master to cleanup/tensor October 13, 2022 21:06
@github-actions
Copy link

@github-actions
Copy link

Array API standard conformance tests for dpctl=0.14.0dev0=py310h8c27c75_179 ran successfully.
Passed: 33
Failed: 801
Skipped: 3138

// Import Cython CAPI for dpctl
import_dpctl();

this->Py_SyclDeviceType_ = &Py_SyclDeviceType;
Copy link
Contributor

Choose a reason for hiding this comment

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

All these are global pointers that are initialized by import_dpctl? We need comments at least here. Moreover, can we create aliases for when we are initializing the static variables SyclDevice_GetDeviceRef etc. that have a name such as gSyclDevice_GetDeviceRef? The idea is to make the code easier to follow.

Copy link
Contributor

@diptorupd diptorupd left a comment

Choose a reason for hiding this comment

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

Overall looks good.

@github-actions
Copy link

Array API standard conformance tests for dpctl=0.14.0dev0=py310h8c27c75_178 ran successfully.
Passed: 33
Failed: 801
Skipped: 3138

@oleksandr-pavlyk oleksandr-pavlyk merged commit 1c77305 into cleanup/tensor Oct 15, 2022
@oleksandr-pavlyk oleksandr-pavlyk deleted the cleanup-tensor-step3 branch October 15, 2022 21:52
@github-actions
Copy link

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

@oleksandr-pavlyk oleksandr-pavlyk mentioned this pull request Oct 17, 2022
3 tasks
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.

2 participants