Skip to content

Expand urContextGetNativeHandle #433

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
Apr 18, 2023
Merged

Expand urContextGetNativeHandle #433

merged 3 commits into from
Apr 18, 2023

Conversation

@jandres742
Copy link

@jandres742 jandres742 commented Apr 10, 2023

Add needed members for PI:

NumDevices
Devices
OwnNativeHandle

@jandres742 jandres742 mentioned this pull request Apr 10, 2023
Closed
@jandres742 jandres742 force-pushed the url0_loader_urContextGetNativeHandle branch 9 times, most recently from 393a2c2 to 6c7f0e3 Compare April 11, 2023 21:05
@jandres742
Copy link
Author

jandres742 commented Apr 11, 2023

@kbenzie : do you know why this is being flagged as an error in format? If I do the change, then python test fails:

--- a/include/ur.py
+++ b/include/ur.py
@@ -1705,9 +1705,9 @@ else:
 ###############################################################################
 ## @brief Function-pointer for urContextCreateWithNativeHandle
 if __use_win_types:
-    _urContextCreateWithNativeHandle_t = WINFUNCTYPE( ur_result_t, ur_native_handle_t, c_ulong, POINTER(ur_device_handle_t), POINTER(ur_context_native_desc_t), POINTER(ur_context_handle_t) )
+    _urContextCreateWithNativeHandle_t = WINFUNCTYPE( ur_result_t, ur_native_handle_t, c_ulong, *, POINTER(ur_context_native_desc_t), POINTER(ur_context_handle_t) )
 else:
-    _urContextCreateWithNativeHandle_t = CFUNCTYPE( ur_result_t, ur_native_handle_t, c_ulong, POINTER(ur_device_handle_t), POINTER(ur_context_native_desc_t), POINTER(ur_context_handle_t) )
+    _urContextCreateWithNativeHandle_t = CFUNCTYPE( ur_result_t, ur_native_handle_t, c_ulong, *, POINTER(ur_context_native_desc_t), POINTER(ur_context_handle_t) )

pbalcer
pbalcer reviewed Apr 12, 2023
View reviewed changes
scripts/core/context.yml Outdated
- type: uint32_t
name: numDevices
desc: "[in] number of devices associated with the context"
- type: "const $x_device_handle_t *"
Copy link
Contributor

@pbalcer pbalcer Apr 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If I had to guess, the python problem is because of the space before the asterisk. This should be "const $x_device_handle_t*" instead.

It might also be a good idea to fix the python generation script, but that's probably a bit more work.

Copy link
Contributor

@kbenzie kbenzie Apr 12, 2023

Choose a reason for hiding this comment

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

This should be "const $x_device_handle_t*" instead.

Agree this looks like it should resolve the issue. I'll have a look at fixing the script separately.

Copy link
Contributor

@kbenzie kbenzie Apr 12, 2023

Choose a reason for hiding this comment

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

This extra space also seems to change the scripts understanding of this type to handle rather than the intended pointer to handle...

Copy link
Author

@jandres742 jandres742 Apr 12, 2023

Choose a reason for hiding this comment

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

thanks @kbenzie , @pbalcer ! yes, it was that :)

@jandres742 jandres742 force-pushed the url0_loader_urContextGetNativeHandle branch 3 times, most recently from faacc56 to 8bbbf3d Compare April 12, 2023 14:20
@jandres742 jandres742 marked this pull request as ready for review April 12, 2023 15:09
@jandres742
Copy link
Author

jandres742 commented Apr 12, 2023

@kbenzie , @pbalcer , @smaslov-intel : please review.

kbenzie
kbenzie approved these changes Apr 12, 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, couple of naming convention things to chagne for consistency.

scripts/core/context.yml Outdated
type: struct
desc: "Descriptor for $xContextCreateWithNativeHandle."
class: $xContext
name: $x_context_native_desc_t
Copy link
Contributor

@kbenzie kbenzie Apr 12, 2023

Choose a reason for hiding this comment

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

In UR these creation structs have been using the *_properties_t naming rather than *_desc_t. Its crossed my mind a few times if that was not the intention of the scripts but for consistency I think this should be *_properties_t.

Copy link
Author

@jandres742 jandres742 Apr 12, 2023

Choose a reason for hiding this comment

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

Good idea, Changing.

scripts/core/context.yml Outdated
name: phDevices
desc: "[in][range(0, numDevices)] list of devices associated with the context"
- type: "const $x_context_native_desc_t*"
name: pContextNativeDesc
Copy link
Contributor

@kbenzie kbenzie Apr 12, 2023

Choose a reason for hiding this comment

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

In other creation entry-points, and in relation to using the *_properties_t suffix, this argument should be called pProperties.

@jandres742 jandres742 force-pushed the url0_loader_urContextGetNativeHandle branch from 76b0386 to 5350d2b Compare April 12, 2023 16:54
smaslov-intel
smaslov-intel approved these changes Apr 12, 2023
View reviewed changes
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

LGTM

Expand urContextGetNativeHandle
d41108a 
Add needed members for PI:

NumDevices
Devices
OwnNativeHandle

Signed-off-by: Jaime Arteaga <jaime.a.arteaga.molina@intel.com>
@jandres742 jandres742 force-pushed the url0_loader_urContextGetNativeHandle branch 2 times, most recently from 17d8520 to a12d475 Compare April 18, 2023 15:08
Change from desc to properties
a12d475 
Signed-off-by: Jaime Arteaga <jaime.a.arteaga.molina@intel.com>
@jandres742
Copy link
Author

jandres742 commented Apr 18, 2023

@kbenzie , @pbalcer : please let me know if there are further comments.

pbalcer
pbalcer approved these changes Apr 18, 2023
View reviewed changes
scripts/core/context.yml Outdated
name: phDevices
desc: "[in][range(0, numDevices)] list of devices associated with the context"
- type: "const $x_context_native_properties_t*"
name: pContextNativeProperties
Copy link
Contributor

@pbalcer pbalcer Apr 18, 2023

Choose a reason for hiding this comment

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

Suggested change
name: pContextNativeProperties
name: pProperties

Copy link
Author

@jandres742 jandres742 Apr 18, 2023

Choose a reason for hiding this comment

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

thanks @pbalcer . Changing it.

@jandres742 jandres742 force-pushed the url0_loader_urContextGetNativeHandle branch from 0aabd6c to 1cfe415 Compare April 18, 2023 17:08
Address comments
1cfe415 
Signed-off-by: Jaime Arteaga <jaime.a.arteaga.molina@intel.com>
@pbalcer pbalcer merged commit 59d9cbc into oneapi-src:main Apr 18, 2023
@kbenzie kbenzie mentioned this pull request Jan 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pbalcer pbalcer pbalcer approved these changes

+2 more reviewers

@kbenzie kbenzie kbenzie approved these changes

@smaslov-intel smaslov-intel smaslov-intel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jandres742 @kbenzie @pbalcer @smaslov-intel