Skip to content

[SYCL][PI] Add interoperability with generic handles to device and program classes #1244

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 8 commits into from
Mar 19, 2020
Merged

[SYCL][PI] Add interoperability with generic handles to device and program classes #1244

merged 8 commits into from
Mar 19, 2020

Conversation

garimagu
Copy link
Contributor

@garimagu garimagu commented Mar 4, 2020

This is an effort to handle interoperability with any SYCL BE.
Currently, the support if only handled for Device and Program class.

@fwyzard
Copy link
Contributor

fwyzard commented Mar 4, 2020

Could you add stubs for the new PI functions to the CUDA backend ?

Or, does the plugin system detect if a function is not implemented in a plugin and already warns accordingly ?

@bader bader changed the title Adding support to use BE raw handles (for device and program) [SYCL][PI] Add interoperability with generic handles to device and program classes Mar 4, 2020
bader
bader reviewed Mar 4, 2020
View reviewed changes
sycl/plugins/opencl/pi_opencl.cpp
assert(program);
assert(handle);

if (*program == nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What dies it mean?
*program type is pi_program.
Probably it's better to re-write:

if (*handle == nullptr) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it means that *program is not initialized.
Even if I chnage it to if(*handle==nullptr), based ont he current code, I will have to add assert(*program==nullptr); So the check would still be there.

@garimagu
Copy link
Contributor Author

garimagu commented Mar 4, 2020

Could you add stubs for the new PI functions to the CUDA backend ?

Or, does the plugin system detect if a function is not implemented in a plugin and already warns accordingly ?

It does not give a warning. I will add the method and use the "not implemented" message you are in pi_cuda.cpp file.
thanks for pointing it out.

@garimagu garimagu force-pushed the rawhandle_interop branch from d12681e to 58a113e Compare March 5, 2020 18:15
romanovvlad
romanovvlad requested changes Mar 6, 2020
View reviewed changes
@garimagu garimagu force-pushed the rawhandle_interop branch from 58a113e to 0991aaf Compare March 10, 2020 21:19
@AGindinson AGindinson requested a review from romanovvlad March 11, 2020 14:23
@garimagu
Copy link
Contributor Author

@smaslov-intel @romanovvlad Let me know if you see anything missing.

@bader
Copy link
Contributor

bader commented Mar 15, 2020

@garimagu, could you update the branch, please?

@smaslov-intel smaslov-intel mentioned this pull request Mar 17, 2020
Merged
romanovvlad
romanovvlad previously approved these changes Mar 17, 2020
View reviewed changes
garimagu added 7 commits March 17, 2020 14:06
@garimagu
[SYCL][PI] Adding support to use raw BE handles (for device and program)
3192bd1 
Signed-off-by: Garima Gupta <[email protected]>
@garimagu
Addition of piProgramRetain and piDeviceRetain that was removed.
929622f 
Signed-off-by: Garima Gupta <[email protected]>
@garimagu
clang-format use on the files.
3f1da30 
Signed-off-by: Garima Gupta <[email protected]>
@garimagu
Addition of corresponding changes to pi_cuda.cpp
368ff75 
Signed-off-by: Garima Gupta <[email protected]>
@garimagu
Addition of suggested changes.
8a11f91 
Signed-off-by: Garima Gupta <[email protected]>
@garimagu
Corrected use of using keyword.
76503f6 
Signed-off-by: Garima Gupta <[email protected]>
@garimagu
[SYCL] Fixing an error in device_impl.cpp. Was using the wrong variable
d2def6e 
Device instead of MDevice.

Signed-off-by: Garima Gupta <[email protected]>
@garimagu garimagu force-pushed the rawhandle_interop branch from 9884379 to baff0b9 Compare March 17, 2020 21:07
@garimagu
[SYCL] Change of name of piextDevice/ProgramInterop to piextDevicePro…
bb7793b 
…gramConvert.

Returning error code from clRetainDevice/clRetainProgram.

Signed-off-by: Garima Gupta <[email protected]>
@garimagu garimagu force-pushed the rawhandle_interop branch from baff0b9 to bb7793b Compare March 17, 2020 21:12
Ruyk
Ruyk reviewed Mar 18, 2020
View reviewed changes
Copy link
Contributor

@Ruyk Ruyk left a comment

Choose a reason for hiding this comment

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

Why specific interop functions per PI type and not just a generic piGetInteropHandle function that can retrieve any? Seems it is easier to maintain if its on a single place. For the OpenCL backend is probably just a cast, and for other backends there may be other ways of converting the type (e.g. for the CUDA backend its likely calling the .get() method on the handle).

@romanovvlad
Copy link
Contributor

Why specific interop functions per PI type and not just a generic piGetInteropHandle function that can retrieve any? Seems it is easier to maintain if its on a single place. For the OpenCL backend is probably just a cast, and for other backends there may be other ways of converting the type (e.g. for the CUDA backend its likely calling the .get() method on the handle).

Let's address this concern separately.

@garimagu
Copy link
Contributor Author

@bader Please merge if you think the changes look ok.

@garimagu
Copy link
Contributor Author

Why specific interop functions per PI type and not just a generic piGetInteropHandle function that can retrieve any? Seems it is easier to maintain if its on a single place. For the OpenCL backend is probably just a cast, and for other backends there may be other ways of converting the type (e.g. for the CUDA backend its likely calling the .get() method on the handle).

Let's address this concern separately.

We can continue the discussion on the review: #1332

@bader bader merged commit 58dbd29 into intel:sycl Mar 19, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Mar 20, 2020
Merge branch 'sycl' into private/abatashe/host_accessor_refactor
a126789 
* sycl: (1209 commits)
  [SYCL] Check exit status get_device_count_by_type
  [SYCL][Doc] Update sub-group extension docs (intel#1330)
  [SYCL][Doc] Add leader to GroupAlgorithms (intel#1297)
  [SYCL] Add SYCL headers search path to default compilation options (intel#1347)
  [SYCL][PI] Add interoperability with generic handles to device and program classes (intel#1244)
  Move SPIR devicelib to top level (intel#1276)
  [SYCL][Driver] Improve fat static library support (intel#1319)
  [SYCL] Remove image_api LIT (intel#1349)
  [SYCL] Fix headers location for check-sycl-deploy target
  [SYCL] Allow gcc asm statements in kernel code (intel#1341)
  [SYCL] Add Intel FPGA force_pow2_depth attribute (intel#1284)
  [SPIR-V][NFC] Fix for building llvm-spirv with -DLLVM_LINK_LLVM_DYLIB=ON (intel#1323)
  [SYCL][NFC] Fix execution graph dump (intel#1331)
  [SYCL][Doc] Release SYCL_INTEL_enqueue_barrier extension document (intel#1199)
  [SYCL][USM] Fix USM malloc_shared and free to handle zero byte (intel#1273)
  [SYCL] Fix undefined symbols in async_work_group_copy (intel#1243)
  [SYCL] Mark calls to barrier and work-item functions as convergent
  [SYCL][CUDA] Fix CUDA plug-in build with enabled assertions (intel#1325)
  [SYCL][Test] Add OpenCL requirement to test/ordered_queue/prop.cpp (intel#1335)
  [SYCL][CUDA] Improve CUDA backend documentation (intel#1293)
  ...
vmaksimo pushed a commit to vmaksimo/llvm that referenced this pull request Oct 18, 2021
@pauzinl @vmaksimo
Store result of fixed point integer SPIR-V instructions wider than 64…
4365160 
… bits (intel#1244)

This if fix for intel#1213
This patch adds Store instructions in cases when return value is wider than 64 bits

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@7904ea9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader left review comments

@Ruyk Ruyk Ruyk left review comments

@fwyzard fwyzard fwyzard left review comments

@smaslov-intel smaslov-intel smaslov-intel left review comments

@romanovvlad romanovvlad romanovvlad 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
@garimagu @fwyzard @bader @romanovvlad @Ruyk @smaslov-intel