Skip to content

[SYCL][Doc] First revision of standard layout relaxation extension #1344

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 2 commits into from
Mar 25, 2020
Merged

[SYCL][Doc] First revision of standard layout relaxation extension #1344

merged 2 commits into from
Mar 25, 2020

Conversation

mkinsner
Copy link

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

First revision of standard layout relaxation extension
c46d59f 
Signed-off-by: Michael Kinsner <michael.kinsner@intel.com>
@mkinsner mkinsner changed the title First revision of standard layout relaxation extension [SYCL][Doc] First revision of standard layout relaxation extension Mar 18, 2020
Pennycook
Pennycook previously approved these changes Mar 18, 2020
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.

LGTM

keryell
keryell suggested changes Mar 19, 2020
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.

That seems like a useful extension.

sycl/doc/extensions/RelaxStdLayout/SYCL_INTEL_relax_standard_layout.asciidoc Outdated

==== To:

Sharing data structures between host and device code imposes certain restrictions, such as use of only user defined classes that are {cpp}11 trivially copyable classes for the data structures, classes that are {cpp}11 trivially copyable classes for the data structures, and in general, no pointers initialized for the host can be used on the device.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would clearer with smaller lines.
For example to see that you can remove the repetition ", classes that are {cpp}11 trivially copyable classes for the data structures,"

Copy link
Author

Choose a reason for hiding this comment

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

Fixed the bug, and will consider wrapping lines :)

Ruyk
Ruyk previously approved these changes Mar 19, 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.

Looks good, is there any way of checking an implementation has support for this extension? macro with the name?

romanovvlad
romanovvlad reviewed Mar 22, 2020
View reviewed changes
sycl/doc/extensions/RelaxStdLayout/SYCL_INTEL_relax_standard_layout.asciidoc Outdated

== Overview

SYCL 1.2.1 required data stored into a buffer or passed as a kernel argument to be standard layout. This is in addition to the data also being trivially copyable. This extension relaxes the standard layout requirement while leaving the trivially copyable requirement intact.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SYCL 1.2.1 required data stored into a buffer or passed as a kernel argument to be standard layout. This is in addition to the data also being trivially copyable. This extension relaxes the standard layout requirement while leaving the trivially copyable requirement intact.
SYCL 1.2.1 requires data stored into a buffer or passed as a kernel argument to be standard layout. This is in addition to the data also being trivially copyable. This extension relaxes the standard layout requirement while leaving the trivially copyable requirement intact.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. Fixed

Incorporate feedback
ebb8d44 
Signed-off-by: Michael Kinsner <michael.kinsner@intel.com>
@mkinsner mkinsner dismissed stale reviews from Ruyk and Pennycook via ebb8d44 March 24, 2020 20:16
@mkinsner
Copy link
Author

Looks good, is there any way of checking an implementation has support for this extension? macro with the name?

We didn't include a specific macro but it is active when the language version macro is >1.2.1. Do you think that this one needs an additional macro? If so I can update the spec.

@mkinsner
Copy link
Author

@bader ready to merge from my perspective

@bader bader requested review from Pennycook, romanovvlad and Ruyk March 25, 2020 05:08
@fwyzard fwyzard mentioned this pull request Mar 25, 2020
Open
@bader bader merged commit ce53521 into intel:sycl Mar 25, 2020
alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Mar 27, 2020
@alexbatashev
Merge remote-tracking branch 'upstream/sycl' into private/abatashe/sp…
6790032 
…hinx

* upstream/sycl: (357 commits)
  [Support] Implement a simple tabular data management library (intel#1358)
  [Support] Implement a property set I/O library (intel#1357)
  [SYCL] Fix buffer constructor using iterators (intel#1386)
  [SYCL][FPGA] Enable a set of loop attributes (intel#1312)
  [Driver][SYCL][FPGA] Proper dependency output location when given /Fo<dir> (intel#1346)
  [SPIR-V] Enabling SPIR-V builtin lookup in device SYCL mode (intel#1384)
  [SYCL][NFC] Unify setting kernel arguments (intel#1379)
  [SYCL][Doc] First revision of standard layout relaxation extension (intel#1344)
  [SYCL] Fixed sub-buffer alloca search (intel#1385)
  [SYCL][FPGA] Emit multiple IR variants for the IVDep attribute (intel#1383)
  [SYCL] Add experimental flag to enable front-end optimizations (intel#1376)
  [SYCL] Remove unexpected double in complex SPIR-V for float support (intel#1381)
  [SYCL] Default work-group sizes based on max (intel#952)
  [SYCL][CUDA] Fix usage of multiple backends in the same program (intel#1252)
  [SPIR-V] Add SPIR-V builtin definitions to the builtin lookup.
  [SPIR-V] Add macro definition when -fdeclare-spirv-builtins is activated
  [SYCL] Fix sycl_generic printing
  [SYCL] Support intel::reqd_work_group_size (intel#1328)
  [SYCL][NFC] Make the RT::PiPlugin object private (intel#1375)
  [SPIRV] Add convergent attribute to SPIR-V built-ins (intel#1373)
  ...
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Feb 23, 2023
@aelovikov-intel
[SYCL] Remove <sycl/sycl.hpp> workaround (intel#1344)
cc4c369
againull pushed a commit that referenced this pull request Feb 15, 2024
@aarongreig
[UR] Pull in fix from UR #1344. (#12725)
90bcc32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keryell keryell keryell requested changes

@Pennycook Pennycook Pennycook approved these changes

@romanovvlad romanovvlad romanovvlad approved these changes

@jbrodman jbrodman Awaiting requested review from jbrodman

@rolandschulz rolandschulz Awaiting requested review from rolandschulz

@Ruyk Ruyk Awaiting requested review from Ruyk

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
@mkinsner @Pennycook @keryell @Ruyk @romanovvlad @bader