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

[SYCL][Doc] Add device global extension spec #4233

Merged
merged 14 commits into from
Sep 29, 2021
Merged

[SYCL][Doc] Add device global extension spec #4233

merged 14 commits into from
Sep 29, 2021

Conversation

mkinsner
Copy link

@mkinsner mkinsner commented Aug 2, 2021

Add device global extension spec

Signed-off-by: Michael Kinsner michael.kinsner@intel.com

[SYCL][Doc] Add device global extension spec
6237869 
Signed-off-by: Michael Kinsner <michael.kinsner@intel.com>
@mkinsner mkinsner added spec extension All issues/PRs related to extensions specifications Documentation Missing documentation for the code, compiler or runtime features, etc. labels Aug 2, 2021
@mkinsner mkinsner requested a review from a team August 2, 2021 04:14
Pennycook
Pennycook reviewed Aug 2, 2021
View reviewed changes
Copy link
Contributor

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

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

A few minor formatting suggestions, but on the whole LGTM.

sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Show resolved Hide resolved
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Show resolved Hide resolved
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Outdated Show resolved Hide resolved
Mike Kinsner and others added 3 commits August 2, 2021 13:19
Pennycook
Pennycook previously approved these changes Aug 3, 2021
View reviewed changes
keryell
keryell reviewed Aug 4, 2021
View reviewed changes
Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Quite useful and simple to use!
This is simpler than https://github.com/triSYCL/triSYCL/blob/master/tests/scope/queue.cpp proposal and the fact you can access globally a variable is very powerful to avoid passing handler or context all over the place.
Do you have also a pure host library implementation not requiring any specific compiler?

sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Show resolved Hide resolved
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Show resolved Hide resolved
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Outdated Show resolved Hide resolved
Mike Kinsner and others added 2 commits August 9, 2021 09:50
@keryell
Update sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asci…
86abbad 
…idoc

Co-authored-by: Ronan Keryell <ronan@keryell.fr>
@keryell
Update sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asci…
08d7911 
…idoc

Co-authored-by: Ronan Keryell <ronan@keryell.fr>
@mkinsner
Copy link
Author

mkinsner commented Aug 9, 2021

Quite useful and simple to use!
This is simpler than https://github.com/triSYCL/triSYCL/blob/master/tests/scope/queue.cpp proposal and the fact you can access globally a variable is very powerful to avoid passing handler or context all over the place.
Do you have also a pure host library implementation not requiring any specific compiler?

Our original forms of the extension were designed to be straightforward to implement with a pure library, but the resultant syntax was making the feature too cumbersome to use (e.g. passing device handler through call trees) and broke the value proposition in multiple cases. We decided to prioritize usability with this version, and removed the language aspects that aided lib-only implementation. It's likely still possible to implement that way but with additional complexity - we concluded that clean language was more important in this feature based on feedback and by looking at the motivating uses.

gmlueck
gmlueck reviewed Aug 13, 2021
View reviewed changes
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Show resolved Hide resolved
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Outdated Show resolved Hide resolved
sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asciidoc Outdated Show resolved Hide resolved
@keryell
Copy link
Contributor

keryell commented Aug 14, 2021

Our original forms of the extension were designed to be straightforward to implement with a pure library, but the resultant syntax was making the feature too cumbersome to use (e.g. passing device handler through call trees) and broke the value proposition in multiple cases. We decided to prioritize usability with this version, and removed the language aspects that aided lib-only implementation. It's likely still possible to implement that way but with additional complexity - we concluded that clean language was more important in this feature based on feedback and by looking at the motivating uses.

That makes sense.
Probably playing with TLS would allow a pure library implementation.

Mike Kinsner and others added 5 commits August 17, 2021 13:25
@gmlueck
Update sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asci…
5b0484b 
…idoc

Co-authored-by: Greg Lueck <gregory.m.lueck@intel.com>
@gmlueck
Update sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asci…
4faf98b 
…idoc

Co-authored-by: Greg Lueck <gregory.m.lueck@intel.com>
@gmlueck
Update sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asci…
9ecd534 
…idoc

Co-authored-by: Greg Lueck <gregory.m.lueck@intel.com>
@gmlueck
Update sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asci…
3d7c287 
…idoc

Co-authored-by: Greg Lueck <gregory.m.lueck@intel.com>
@gmlueck
Update sycl/doc/extensions/DeviceGlobal/SYCL_INTEL_device_global.asci…
82122bf 
…idoc

Co-authored-by: Greg Lueck <gregory.m.lueck@intel.com>
@jbrodman jbrodman self-requested a review August 20, 2021 18:43
jbrodman
jbrodman previously approved these changes Aug 23, 2021
View reviewed changes
@bader
Copy link
Contributor

bader commented Sep 1, 2021

It looks like there a few comments from @gmlueck to address. @mkinsner, @gmlueck, am I right?

@gmlueck
Copy link
Contributor

gmlueck commented Sep 1, 2021

It looks like there a few comments from @gmlueck to address. @mkinsner, @gmlueck, am I right?

Yes, and we agreed recently on a few more changes too. @mkinsner asked me to make the changes myself, so I will create a PR against this PR, and mike will merge my PR into his. After that, we can merge mike's PR into the mainline.

@bader
Copy link
Contributor

bader commented Sep 1, 2021

Thanks for the update.
Just in case it might be useful, I think you should be able to commit directly to https://github.com/mkinsner/llvm/tree/private/mkinsner/device_global_extension.

@gmlueck
Address my remaining review comments
3e04b44 
@mkinsner told me he agrees with these comments and asked me to make
them myself.
@gmlueck
Copy link
Contributor

gmlueck commented Sep 24, 2021

It looks like there a few comments from @gmlueck to address. @mkinsner, @gmlueck, am I right?

@mkinsner told me verbally that he agrees with my comments and asked me to make them myself. I did this in 3e04b44.

I approve this PR now, and it can be merged unless others have concerns.

gmlueck
gmlueck previously approved these changes Sep 24, 2021
View reviewed changes
@bader bader requested a review from jbrodman September 24, 2021 15:06
@gmlueck
Copy link
Contributor

gmlueck commented Sep 28, 2021

@olegmaslovatintel: Can someone merge this PR while @bader is on vacation? I think all outstanding issues have been resolved.

@olegmaslovatintel
Copy link

@vladimirlaz @pvchupin could you please merge?

@vladimirlaz vladimirlaz merged commit d3e70d4 into intel:sycl Sep 29, 2021
alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Oct 3, 2021
@alexbatashev
Merge branch 'sycl' into remove_outdated_templates
00da882 
* sycl: (108 commits)
  [SYCL][XPTI] Revisit resource management strategy (intel#4494)
  [SYCL][ESIMD] Fix misprint: ESIMD_L1_FLUASH_RO_DATA -> ESIMD_L1_FLUSH_RO_DATA (intel#4681)
  [SYCL] Make kernel_bundle interop more conformant (intel#4672)
  [SYCL] Submission with kernel parameter ignores set kernel bundle (intel#4667)
  [SYCL] Add support for std::byte to vec class  (intel#4637)
  [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process (intel#4671)
  [SYCL] Fix an error on host when big image is used on opencl:gpu (intel#4668)
  [SYCL] Exclude exported symbols from kernel bundles (intel#4660)
  Revert "[SYCL] Allow overriding plugin libraries (intel#4067)" (intel#4659)
  [SYCL] Handle exceptions on mutually exclusive handler operations (intel#4639)
  [sycl-post-link] Don't split module if function pointer has a user that's not CallInst (intel#4657)
  [SYCL][HIP] Fix MemBufferFill for nvidia platform (intel#4629)
  [SYCL][Doc] Describe DPC++ CUDA install w/ non-standard toolkit loc (intel#4663)
  [SYCL] Fix device code instrumentation (intel#4615)
  Remove myself as a code owner (intel#4653)
  [SYCL] Fix overwriting insert to sub_group_mask (intel#4656)
  [x86][Matrix] Replace packed_a with row_major in matrix testcases (intel#4641)
  [SYCL][Doc] Add device global extension spec (intel#4233)
  [sycl-post-link] Adds property listing exported functions (intel#4626)
  [Driver][SYCL] Do not consider non-archive files for FPGA binary checks (intel#4644)
  ...
alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Oct 3, 2021
@alexbatashev
Merge branch 'sycl' into sycl_compile_perf
a4f4c4d 
* sycl: (107 commits)
  [SYCL][XPTI] Revisit resource management strategy (intel#4494)
  [SYCL][ESIMD] Fix misprint: ESIMD_L1_FLUASH_RO_DATA -> ESIMD_L1_FLUSH_RO_DATA (intel#4681)
  [SYCL] Make kernel_bundle interop more conformant (intel#4672)
  [SYCL] Submission with kernel parameter ignores set kernel bundle (intel#4667)
  [SYCL] Add support for std::byte to vec class  (intel#4637)
  [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process (intel#4671)
  [SYCL] Fix an error on host when big image is used on opencl:gpu (intel#4668)
  [SYCL] Exclude exported symbols from kernel bundles (intel#4660)
  Revert "[SYCL] Allow overriding plugin libraries (intel#4067)" (intel#4659)
  [SYCL] Handle exceptions on mutually exclusive handler operations (intel#4639)
  [sycl-post-link] Don't split module if function pointer has a user that's not CallInst (intel#4657)
  [SYCL][HIP] Fix MemBufferFill for nvidia platform (intel#4629)
  [SYCL][Doc] Describe DPC++ CUDA install w/ non-standard toolkit loc (intel#4663)
  [SYCL] Fix device code instrumentation (intel#4615)
  Remove myself as a code owner (intel#4653)
  [SYCL] Fix overwriting insert to sub_group_mask (intel#4656)
  [x86][Matrix] Replace packed_a with row_major in matrix testcases (intel#4641)
  [SYCL][Doc] Add device global extension spec (intel#4233)
  [sycl-post-link] Adds property listing exported functions (intel#4626)
  [Driver][SYCL] Do not consider non-archive files for FPGA binary checks (intel#4644)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artemrad artemrad artemrad left review comments

@AaronBallman AaronBallman AaronBallman left review comments

@keryell keryell keryell left review comments

@gmlueck gmlueck gmlueck approved these changes

@jbrodman jbrodman jbrodman approved these changes

@Pennycook Pennycook Awaiting requested review from Pennycook

Assignees
No one assigned
Labels
Documentation Missing documentation for the code, compiler or runtime features, etc. spec extension All issues/PRs related to extensions specifications
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@mkinsner @keryell @bader @gmlueck @olegmaslovatintel @Pennycook @jbrodman @AaronBallman @artemrad @vladimirlaz