Skip to content

[SYCL] Add missing builtins for vec<X, 1> #1984

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

Merged
merged 6 commits into from
Jun 30, 2020

Conversation

alexbatashev
Copy link
Contributor

On host using builtins with vec<X, 1> types causes linkage errors. Add missing overloads.

@alexbatashev alexbatashev requested a review from a team as a code owner June 25, 2020 09:19
@alexbatashev alexbatashev requested a review from v-klochkov June 25, 2020 09:19
@v-klochkov
Copy link
Contributor

Please add a trivial LIT test or extend some existing test.

@s-kanaev
Copy link
Contributor

@alexbatashev ping

Alexander Batashev added 2 commits June 29, 2020 22:04
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
@alexbatashev
Copy link
Contributor Author

Please add a trivial LIT test or extend some existing test.

Our current tests are not really scalable, and covering >500 builtins in one PR looks to much for me. Is it ok if I add a simple smoke test for now, and do a followup with better testing?

@bader
Copy link
Contributor

bader commented Jun 29, 2020

Is it ok if I add a simple smoke test for now, and do a followup with better testing?

I think adding a simple smoke test is enough for LIT testing, we are not aiming to cover 100% of use cases with LIT tests.
Better testing can be provided by SYCL-CTS and/or separate test suite.

@alexbatashev
Copy link
Contributor Author

Is it ok if I add a simple smoke test for now, and do a followup with better testing?

I think adding a simple smoke test is enough for LIT testing, we are not aiming to cover 100% of use cases with LIT tests.
Better testing can be provided by SYCL-CTS and/or separate test suite.

Then I’ll look into why CTS didn’t catch missing builtins.

@v-klochkov v-klochkov self-requested a review June 30, 2020 00:40
Copy link
Contributor

@v-klochkov v-klochkov left a comment

Choose a reason for hiding this comment

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

The test needs minor fix in the compilation RUN line.

Alexander Batashev added 2 commits June 30, 2020 10:06
Signed-off-by: Alexander Batashev <[email protected]>
Signed-off-by: Alexander Batashev <[email protected]>
@alexbatashev
Copy link
Contributor Author

@v-klochkov I found even more missing builtins, and I believe some are still missing. I'll do a followup on adding the rest of them once proper testing is in place.

@bader bader merged commit 073a36b into intel:sycl Jun 30, 2020
@alexbatashev alexbatashev deleted the fix_vec_float_scalar branch September 17, 2021 06:46
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