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.

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

Proper DpctlSyclQueue support #963

Merged
merged 2 commits into from
Apr 11, 2023
Merged

Proper DpctlSyclQueue support #963

merged 2 commits into from
Apr 11, 2023

Conversation

chudur-budur
Copy link
Contributor

@chudur-budur chudur-budur commented Mar 7, 2023 •
edited by diptorupd
Loading

Re-implementation of the DpctlSyclQueue type in numba-dpex.

  • The DpctlSyclQueue is no longer a numba OpaqueType. It is reimplemented as a StructType that stores the native queue_ref pointer and the parent PyObject pointer.
  • Introduce C runtime helper functions for boxing/unboxing a dpctl.SyclQueue

Fixes #930

  • 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?

@chudur-budur chudur-budur self-assigned this Mar 9, 2023
@chudur-budur chudur-budur force-pushed the github-930 branch 4 times, most recently from 5789133 to 5b176d0 Compare March 24, 2023 07:58
@diptorupd diptorupd added this to the 0.20.1 milestone Mar 28, 2023
@diptorupd diptorupd marked this pull request as ready for review March 31, 2023 17:04
@diptorupd
Copy link
Contributor

@mingjie-intel can you please review?

@AlexanderKalistratov
Copy link
Contributor

I don't get the exact purpose of this PR. Does it adds some new functionality?
If so, do we have tests for this functionality?
Or this is just refactoring and functionality is going to be added later?

Anyway, I don't see any tests for implemented code. E.g. queue_ref added, but no tests for it.

@diptorupd diptorupd force-pushed the github-930 branch 2 times, most recently from 7c9a4ff to 0160b3b Compare March 31, 2023 17:57
mingjie-intel
mingjie-intel approved these changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@mingjie-intel mingjie-intel 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.

@diptorupd
Copy link
Contributor

diptorupd commented Apr 1, 2023 •
edited
Loading

I don't get the exact purpose of this PR. Does it adds some new functionality? If so, do we have tests for this functionality? Or this is just refactoring and functionality is going to be added later?

At this point there are no user-visible changes. It only changes the implementation inside numba-dpex of the DpctlSyclQueue type. Instead of defining the type as a PyObject we now use a Numba struct type.

The next set of changes to use the new type inside numba-dpex and avoid extra queue creation will be the next PR. The changes here should also make it simpler to implement the changes in your dpnp overload PR.

Anyway, I don't see any tests for implemented code. E.g. queue_ref added, but no tests for it.

I am working on adding a test case.

@fcharras fcharras mentioned this pull request Apr 6, 2023
Merged
@diptorupd diptorupd force-pushed the github-930 branch 2 times, most recently from e0ddad8 to 9f06447 Compare April 8, 2023 07:09
@diptorupd
Copy link
Contributor

I am working on adding a test case.

@AlexanderKalistratov I have added a unit test. Could you please review again? The test evaluates if the queue_ref stored in the native representation of a dpctl.SyclQueue can be properly accessed inside numba-dpex and then passed to a dpctl C function from inside the LLVM module.

Diptorup Deb added 2 commits April 8, 2023 02:12
Reimplementation of the DpctlSyclQueue type in numba-dpex.
1bc474d 
   - The DpctlSyclQueue is no longer a numba OpaqueType. It is
     reimplemented as a StructType that stores the native queue_ref
     pointer and the parent PyObject pointer.
   - Introduce C runtime helper functions for boxing/unboxing a
     dpctl.SyclQueue
Add a unit test to check usability of DpctlSyclQueue type.
32434cc 
   - Added a test to see if the Numba native object created
     for a dpctl.SyclQueue is usable inside the compiler.

     The test extracts the queue_ref pointer from the dpctl.SyclQueue
     PyObject during unboxing and stores it in a native struct.
     The test verifies that the queue_ref pointer is valid and can
     be passed to a C function call generated in the compiled LLVM module.
@diptorupd diptorupd merged commit 46c09ff into IntelPython:main Apr 11, 2023
@diptorupd diptorupd deleted the github-930 branch April 11, 2023 15:28
github-actions bot added a commit that referenced this pull request Apr 11, 2023
@github-actions
Merge pull request #963 from chudur-budur/github-930
3ab8e23 
Proper DpctlSyclQueue support 46c09ff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mingjie-intel mingjie-intel mingjie-intel approved these changes

@AlexanderKalistratov AlexanderKalistratov AlexanderKalistratov approved these changes

@diptorupd diptorupd Awaiting requested review from diptorupd

Assignees

@chudur-budur chudur-budur

Labels
None yet
Projects
None yet
Milestone
0.20.1
Development

Successfully merging this pull request may close these issues.

Properly support dpctl.SyclQueue as a data type inside numba-dpex
4 participants
@chudur-budur @diptorupd @AlexanderKalistratov @mingjie-intel