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.

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

Editorial fix in vector section #522

Merged
merged 2 commits into from
Jan 4, 2024
Merged

Editorial fix in vector section #522

merged 2 commits into from
Jan 4, 2024

Conversation

mkinsner
Copy link

This fix makes the wording better, but I'm actually not sure what it means to have compatible support on the host when device support isn't possible. Should the later part of this sentence be dropped totally, since I don't think there is alternate host emulation? There is independently compatible support for the vector types on the host, unrelated to what the device supports, AFAICT.

down to a <<backend>> built-in vector types on SYCL devices, where possible, and
provides compatible support on the host or when it is not possible.
down to a <<backend>> built-in vector type on SYCL devices, where possible, and
provides compatible support on the host when not possible.
Copy link
Contributor

Choose a reason for hiding this comment

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

This entire sentence seems like an implementation detail to me. How could you write a test for this sentence? It just says that it might or might not use a built-in vector type in device code.

The part about "provides compatible support on the host when not possible" seems not quite accurate. The vec type works on the host regardless of how vec is implemented in device code. So, I think this part of the sentence is just saying that vec is also available in host code. We say that already in the first sentence of the first paragraph in this section.

I think we should just delete the sentence, rather than trying to fix it.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed. The PR should now reflect this

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Can you update the description of this PR also?

@gmlueck
Copy link
Contributor

gmlueck commented Jan 3, 2024

It would be nice to have a couple other company reviewers on this small PR.

@tomdeakin
Copy link
Contributor

WG approves merge

@gmlueck gmlueck merged commit 3f9aae0 into SYCL-2020/master Jan 4, 2024
3 checks passed
keryell pushed a commit that referenced this pull request Sep 10, 2024
gmlueck added a commit that referenced this pull request Nov 7, 2024
Editorial fix in vector section

(cherry picked from commit 3f9aae0)
gmlueck added a commit that referenced this pull request Nov 7, 2024
Editorial fix in vector section

(cherry picked from commit 3f9aae0)
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.

5 participants