Skip to content

Conversation

@AlexeySachkov
Copy link
Contributor

No description provided.

@AlexeySachkov AlexeySachkov requested review from a team and lbushi25 October 9, 2024 13:16
@AlexeySachkov AlexeySachkov requested a review from a team as a code owner October 9, 2024 13:16
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Small nits, otherwise LGTM!


List of data types which should be covered:
- some basic C++ data types of different sizes: `char`, `int16_t`, `int`,
`double`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is double part of the list above or a separate type to be covered? Since half is mentioned by itself, I think it would make sense to do that for double too. Should we also mention that the tests for double and half require aspect::fp64 and aspect::fp16 respectively?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is double part of the list above or a separate type to be covered? Since half is mentioned by itself, I think it would make sense to do that for double too.

half is in a separate list, because it is not a basic C++ type, its a custom type introduced by SYCL specification.

I don't think that we should outline "optional" data types separately, because then we also need to consider atomic_ref for 64-bit types and I generally expect that the fact that certain tests have "extra" requirements and may be skipped on certain HW as implied, simply because that is automatically the case if a test is properly written in accordance with the SYCL specification.

Copy link
Contributor

@lbushi25 lbushi25 left a comment

Choose a reason for hiding this comment

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

LGTM

@AlexeySachkov AlexeySachkov merged commit 27e1089 into intel:sycl Oct 30, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants