Skip to content

[SYCL][Docs] Add design document for device aspect traits #8182

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
Feb 20, 2023
Merged

[SYCL][Docs] Add design document for device aspect traits #8182

merged 8 commits into from
Feb 20, 2023

Conversation

steffenlarsen
Copy link
Contributor

This commit adds a design document for the any_device_has and all_devices_have SYCL 2020 traits.

@steffenlarsen
[SYCL][Docs] Add design document for device aspect traits
e2e02e2 
This commit adds a design document for the `any_device_has` and
`all_devices_have` SYCL 2020 traits.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen requested review from maarquitos14, mdtoguchi and a team February 2, 2023 12:39
@steffenlarsen steffenlarsen requested a review from a team as a code owner February 2, 2023 12:39
@steffenlarsen
Add design doc to TOC
2b7bc7a 
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen requested a review from a team as a code owner February 2, 2023 13:17
@steffenlarsen
Address Greg's offline comments
0d2ff49 
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen
Copy link
Contributor Author

Had a discussion with @gmlueck offline about this design. 0d2ff49 is based on the feedback.

gmlueck
gmlueck approved these changes Feb 6, 2023
View reviewed changes
Copy link
Contributor

@gmlueck gmlueck left a comment

Choose a reason for hiding this comment

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

I think this is fine, but I added a comment about something that seemed a little confusing to me.

sycl/doc/design/DeviceAspectTraitDesign.md Outdated
must assume that there exists some device that supports any given aspect. Since
the driver has no way of knowing all possible aspects, we use a catch-all macro
to denote this case instead. This is not needed for $A^{all}_t$ for any target
$t$, as the driver will always know all relevant aspects.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is kind of a minor point, but the last sentence seems a little confusing to me. It seems to me that the reason we do not need any special macro for the $A^{all}_t$ case is because $A^{all}_t$ is the empty set when there is no configuration entry for t.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re-reading it, I completely agree. I have clarified it. Do you think it is clearer now?

@steffenlarsen
Clarify note about catch-all macros
938de94 
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen
Fix previous clarification
35e5487 
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
mdtoguchi
mdtoguchi reviewed Feb 8, 2023
View reviewed changes
sycl/doc/design/DeviceAspectTraitDesign.md
${\bigcup}^n_{k=1} A^{any}_{tk}$ is the set of all aspects.
* `__SYCL_ANY_DEVICE_HAS_`$j$`__` as `1` for all $j$ in
${\bigcup}^n_{k=1} A^{any}_{tk}$ if `__SYCL_ANY_DEVICE_HAS_ANY_ASPECT__` was not
defined.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me - would these macros be set for device compilation only? Given usage, it does seem to be the case, but maybe call it out for clarity.

Copy link
Contributor

Choose a reason for hiding this comment

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

They should be set for both host and device compilations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried clarifying this in 8f15db0.

AlexeySachkov
AlexeySachkov reviewed Feb 9, 2023
View reviewed changes
@steffenlarsen
Clarify macros being defined on both host and device
8f15db0 
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen
Add comment about optional config sub-key
114a834 
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
maarquitos14
maarquitos14 approved these changes Feb 10, 2023
View reviewed changes
Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

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

LGTM

@steffenlarsen
Amend case for undefined macros
93a5e80 
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen
Copy link
Contributor Author

@maarquitos14 pointed out that the current design does not work if the the driver-controlled macros are undefined. As such the device headers should make sure they are defined if the driver does not define them. This has been amended in 93a5e80.

@gmlueck
Copy link
Contributor

gmlueck commented Feb 17, 2023

@maarquitos14 pointed out that the current design does not work if the the driver-controlled macros are undefined. As such the device headers should make sure they are defined if the driver does not define them. This has been amended in 93a5e80.

Just curious ... why didn't the previous design work? Isn't +0 the same as 0?

@steffenlarsen
Copy link
Contributor Author

@maarquitos14 pointed out that the current design does not work if the the driver-controlled macros are undefined. As such the device headers should make sure they are defined if the driver does not define them. This has been amended in 93a5e80.

Just curious ... why didn't the previous design work? Isn't +0 the same as 0?

It only worked if the macros were defined but without a value. When they were not defined, the preprocessor wouldn't think of them as macros and would leave them in.

@maarquitos14 maarquitos14 mentioned this pull request Feb 17, 2023
Merged
@steffenlarsen steffenlarsen merged commit ec34869 into intel:sycl Feb 20, 2023
steffenlarsen pushed a commit that referenced this pull request Feb 24, 2023
@maarquitos14 @bader
[SYCL] Implements device headers part of any_device_has/`all_device…
81baa50 
…s_have` (#8392)

Following the design document in #8182, these changes implement the
changes required by section `Changes to the device headers`.

---------

Signed-off-by: Maronas, Marcos <marcos.maronas@intel.com>
Co-authored-by: Alexey Bader <alexey.bader@intel.com>
callumfare pushed a commit to callumfare/llvm that referenced this pull request Feb 28, 2023
@steffenlarsen @callumfare
[SYCL][Docs] Add design document for device aspect traits (intel#8182)
4dd4a11 
This commit adds a design document for the `any_device_has` and
`all_devices_have` SYCL 2020 traits.

---------

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmlueck gmlueck gmlueck approved these changes

@AlexeySachkov AlexeySachkov AlexeySachkov left review comments

@bader bader bader approved these changes

@maarquitos14 maarquitos14 maarquitos14 approved these changes

@mdtoguchi mdtoguchi mdtoguchi 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
@steffenlarsen @gmlueck @bader @AlexeySachkov @maarquitos14 @mdtoguchi