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

a few fixes (thread safety & cl_khr_command_buffer UB) #1840

Merged
merged 3 commits into from
Oct 8, 2024

Conversation

franz
Copy link
Contributor

@franz franz commented Nov 4, 2023

some fixes we've been carrying in our CTS fork:

  • fix UB in command_buffer_event_sync.cpp: enqueue of two commands in two separate queues, with both using the same buffer argument, and no synchronization between the commands.
  • fix UB in command_buffer_test_barrier.cpp: missing synchronization between zeroing command and command-buffer using two separate queues
  • make test_thread_dimensions.cpp thread-safe to avoid spurious errors.

@franz
Copy link
Contributor Author

franz commented Dec 7, 2023

Removed the first two commits from the PR; i did not realize there was already an open PR with those.

@franz franz changed the title a few fixes (mostly subgroups & cl_khr_command_buffer) a few fixes (thread safety & cl_khr_command_buffer UB) Jan 16, 2024
@franz franz force-pushed the cts_fixes branch 2 times, most recently from 0309e43 to 8f4b503 Compare January 17, 2024 11:43
@franz
Copy link
Contributor Author

franz commented Jan 18, 2024

rebased on main. should be ready for review.

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

Note, this needs another merge/rebase now, to pick up the latest header changes.

…rrier.cpp

BarrierWithWaitListKHR::Run in command_buffer_test_barrier.cpp contains a
"check second enqueue" which tries to zero out the cl_mems before enqueueing
the command buffer again. However, the zeroing commands are enqueued to the
in-order queue, while the command buffer to the out-of-order queue. Since there
is no implicit or explicit sync, this is undefined behaviour.
…ync.cpp

RunCombufWaitForSecQueueCombuf() is launching two clEnqueueFillBuffer commands
on two separate queues, however both had the same buffer argument, and there
was no synchronization between the commands.

* add an event dependency to the second FillBuffer to ensure correct execution
* change the fill pattern of the first FillBuffer to a different pattern
replaced static variables with stack variables
@bashbaug
Copy link
Contributor

bashbaug commented Oct 8, 2024

Merging as discussed in the October 8th teleconference.

@bashbaug bashbaug merged commit c40c8d5 into KhronosGroup:main Oct 8, 2024
7 checks passed
@franz franz deleted the cts_fixes branch October 18, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants