Implement printf #565
Implement printf #565
Conversation
There was a problem hiding this comment.
Thanks! We can leave removing the serialisation or adding support for batching to a future PR but there are a few things we should IMO fix as part of this one. Ideally we'd add a few tests (I'm thinking of exercising the case where we wrap-around the buffer at least) but I'm fine with deferring to a separate PR if you want.
FWIW, it's passing all the printf conformance tests at my end and the validation layers do not report anything.
| CHECK_RETURN cl_int do_post_action() override final; | ||
|
|
||
| bool can_be_batched() const override final { | ||
| return !m_kernel->uses_printf() && |
There was a problem hiding this comment.
Fine as a first step but we should be able to support batching.
| auto bytes_written = read_inc_buff<uint32_t>(data) * 4; | ||
| auto* data_start = data; | ||
|
|
||
| while (static_cast<size_t>(data - data_start) < bytes_written && |
There was a problem hiding this comment.
Does this mean that we never print anything if the kernel filled the whole buffer and we wrapped around (I haven't checked the clspv side)?
There was a problem hiding this comment.
I have added a test showing that we print as much as possible.
Clspv provides a printf definition that stores the value of each argument in a program-wide printf storage buffer. This implementation uses the appropriate ClspvReflection instructions to parse this data and print the resulting strings at runtime.
Co-authored-by: Kévin Petit <[email protected]>
Co-authored-by: Kévin Petit <[email protected]>
Co-authored-by: Kévin Petit <[email protected]>
Co-authored-by: Kévin Petit <[email protected]>
|
As you proposed I have left removing the serialisation and adding support for batching to a future PR. I tried to address every other comment. And I have added some tests. Especially to convert to wrapping behaviour. |
| #include "spirv/unified1/NonSemanticClspvReflection.h" | ||
|
|
There was a problem hiding this comment.
| #include "spirv/unified1/NonSemanticClspvReflection.h" |
Should no longer be necessary.
| if (m_kernel->uses_printf()) { | ||
| auto buffer = m_queue->get_printf_buffer(); | ||
| if (buffer == nullptr) { | ||
| cvk_error_fn("printf buffer was not created"); |
There was a problem hiding this comment.
I'm not sure this makes sense as a user-visible error. The only to hit this is if we made a mistake in clvk. Maybe an assert would have been enough/more appropriate.
| if (!m_printf_buffer) { | ||
| return nullptr; | ||
| } |
| static void releaseStdout(int fd) { | ||
| fflush(stdout); | ||
| dup2(fd, fileno(stdout)); | ||
| close(fd); | ||
| } | ||
|
|
||
| static bool getStdout(int& fd) { | ||
| fd = dup(fileno(stdout)); | ||
| if (!freopen(stdoutFileName.c_str(), "w", stdout)) { | ||
| fprintf(stderr, "ERROR!\n"); | ||
| releaseStdout(fd); | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| static char* getStdoutContent() { | ||
| FILE* f; | ||
| memset(stdoutBuffer, 0, BUFFER_SIZE); | ||
| fflush(stdout); | ||
| f = fopen(stdoutFileName.c_str(), "r"); | ||
| if (f == nullptr) | ||
| return nullptr; | ||
|
|
||
| char* ptr = stdoutBuffer; | ||
| do { | ||
| ptr += strlen(ptr); | ||
| ptr = fgets(ptr, BUFFER_SIZE, f); | ||
| } while (ptr != nullptr); | ||
| fclose(f); | ||
|
|
||
| return stdoutBuffer; | ||
| } |
There was a problem hiding this comment.
We should introduce a callback to get access to the output of printf from unit tests. As discussed, I want to make this standard in a future version of OpenCL, similarly to how we've done it in the cl_arm_printf extension.
| if (m_kernel->program()->uses_printf()) { | ||
| // Create and initialize the printf buffer | ||
| auto buffer = m_queue->get_or_create_printf_buffer(); | ||
| auto err = m_queue->reset_printf_buffer(); |
There was a problem hiding this comment.
This means we can be writing to the printf buffer concurrently to executing kernels (or maybe even the post-action code) with no protection whatsoever.
|
I've created #578 to track further work. |
Clspv provides a printf definition that stores the value of each argument in a program-wide printf storage buffer. This implementation uses the appropriate ClspvReflection instructions to parse this data and print the resulting strings at runtime.
This is a rebase of #420 + some fixes