Skip to content

[SYCL][LIBCLC] Change __clc_size_t to unsigned #4784

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 4 commits into from
Oct 19, 2021
Merged

[SYCL][LIBCLC] Change __clc_size_t to unsigned #4784

merged 4 commits into from
Oct 19, 2021

Conversation

AidanBeltonS
Copy link
Contributor

This changes the __clc_size_t from a signed 64-bit int to an unsigned 64-bit int.
The change results in the correct mangled name for GroupAsyncCopy built-ins.

This is a proposed solution to issue #4502

@AidanBeltonS AidanBeltonS requested a review from bader as a code owner October 19, 2021 09:13
bader
bader requested changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

I should change the definition on line 123.

@bader bader requested a review from Naghasan October 19, 2021 09:21
@bader bader added CTS Impacts Khronos SYCL CTS libclc libclc project related issues labels Oct 19, 2021
@bader bader linked an issue Oct 19, 2021 that may be closed by this pull request
CTS group and nd_item category compfail 'Unresolved extern function '_Z22__spirv_GroupAsyncCopyjPU3AS1DF16_PU3AS3KDF16_mm9ocl_event'' with latest SYCL build and CUDA #4502
Closed
bader
bader previously approved these changes Oct 19, 2021
View reviewed changes
@bader
Copy link
Contributor

bader commented Oct 19, 2021

It would be great to add a lightweight regression test to pre-commit tests.

@AidanBeltonS
Copy link
Contributor Author

It would be great to add a lightweight regression test to pre-commit tests.

I am not 100% sure on what the pre-commit tests run, is it the llvm-test-suite?
Thanks

@bader
Copy link
Contributor

bader commented Oct 19, 2021

It would be great to add a lightweight regression test to pre-commit tests.

I am not 100% sure on what the pre-commit tests run, is it the llvm-test-suite? Thanks

We run check-llvm, check-clang and check-sycl unconditionally, so if could add "compile-only" test to check-sycl test it would be perfect.
llvm-test-suite can be executed on CUDA machine on demand due to limited resources, so adding test there works as well to some extent.

@AidanBeltonS AidanBeltonS requested a review from a team as a code owner October 19, 2021 13:05
Naghasan
Naghasan approved these changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@Naghasan Naghasan left a comment

Choose a reason for hiding this comment

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

LGTM

@bader bader merged commit c2ccc43 into intel:sycl Oct 19, 2021
@AidanBeltonS AidanBeltonS deleted the fix_group_async_copy branch October 19, 2021 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader approved these changes

@Naghasan Naghasan Naghasan approved these changes

@sergey-semenov sergey-semenov sergey-semenov approved these changes

Assignees
No one assigned
Labels
CTS Impacts Khronos SYCL CTS libclc libclc project related issues
Projects
None yet
Milestone
No milestone
4 participants
@AidanBeltonS @bader @Naghasan @sergey-semenov