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

Add missing wait statements to CCA testing code #642

Merged

Conversation

stephenswat
Copy link
Member

The current CCA testing code for CUDA and SYCL is missing some necessary wait statements, which are causing me issues in #609. This commit fixes the problem by adding the missing waits.

@stephenswat stephenswat added bug Something isn't working tests Make sure the code keeps working cuda Changes related to CUDA sycl Changes related to SYCL labels Jul 16, 2024
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

I'm very surprised about your conclusion on this one. 🤔 Why should we need to wait on these operations? If anything, I would conclude that the setup(...) calls were missing ignore() calls. 🤔

These copies should be fine to be put into the same stream as the kernels that get launched after them. Why do we need synchronization points on them? 😕

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Ahh, never mind. The SYCL versions do need to be synchronized I guess. Since sycl::queue-s don't work quite in the same way as a CUDA stream. (Events are not necessarily ordered in them.) So the CUDA test could technically would just fine without these changes. It's only the SYCL test that is more finnicky. 🤔

But since we're only talking about some unit tests here, uniformity is probably more important to have.

The current CCA testing code for CUDA and SYCL is missing some necessary
wait statements, which are causing me issues in acts-project#609. This commit fixes
the problem by adding the missing waits.
@stephenswat stephenswat force-pushed the fix/cca_test_missing_waits branch from b19d265 to 90f85e3 Compare July 16, 2024 13:56
@stephenswat stephenswat enabled auto-merge July 16, 2024 13:57
@stephenswat stephenswat merged commit 4377873 into acts-project:main Jul 16, 2024
23 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cuda Changes related to CUDA sycl Changes related to SYCL tests Make sure the code keeps working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants