google
Implement printf
Add support for printf. The compiler will generate definitions that store a unique identifier for the given format string and the value of each argument.
A storage buffer of a configurable size is used to provide the printf buffer. Information on the buffer and the individual printf calls is embedded in the reflection instructions.
Also add a few misc fixes to make this work:
- Allow lowering of ConstantExprs assuming there is only 1 user
- Add support for atomic instruction implicit GEPs to ReplacePointerBitcastPass
Related Pull Requests (the SPIRV-Headers change is required): https://github.com/KhronosGroup/SPIRV-Headers/pull/290 https://github.com/KhronosGroup/SPIRV-Registry/pull/168 https://github.com/kpet/clvk/pull/420/
This contribution is being made by Codeplay on behalf of Samsung.
Can you describe the overall design succinctly in one of these pull requests? My understanding is the following:
- clspv creates a descriptor for a storage buffer for printf and uses index 0 in the buffer as an atomic counter for reservations
- Every printf call is translated as a reservation of space for an id of the call and the size of arguments. The id is unique to the format string.
- The format strings are reflected by the PrintInfo reflection instruction. Clvk is expected map these ids and strings per kernel/program to translate the recorded data into actual printf statements on the host.
- Clvk needs to manage a buffer for the data. It is not clear how many buffers are needed. Maybe just one per queue, but reuse of the buffer might be problematic.
@kpet any thoughts? The clspv side of things seems reasonable if I'm understanding correctly, but the buffer management seems like it might be a pain.
Can you describe the overall design succinctly in one of these pull requests? My understanding is the following:
I've added some documentation on the design to docs/OpenCLCOnVulkan.md - I'm happy to add to this if it's still not clear.
Your understanding is basically correct, although the printf ID is unique to the printf call site rather the format string. Multiple printf calls with the same format string (but possibly different arguments) will have unique IDs.
There's some ongoing discussion on the clvk pull request on the best way to handle the buffer.
I've updated this PR, but to reduce future merge conflicts and general noise it's now based on top of the physical constant address space PR - #974 - so the first commit should be ignored in the review
Once that gets merged, this can be rebased on top of main
I've rebased and updated this. Unfortunately there's now a regression with the printf_physical_buffer.cl lit test - it seems like some recent changes mean that with physical addressing enabled, the printf buffer in the final SPIR-V has the type i32 instead of { [0 x i32] }, so the index operands don't match. I'm guessing this is related to opaque pointer support.
Unfortunately we no longer have time to work on this, so it would be great if someone else could pick this up. I suspect it would be a fairly quick/small fix for anyone who is more familiar with the recent changes for opaque pointers etc. I believe everything else is still working correctly, and the clvk changes are also ready to go.
Thanks @callumfare. Samsung side will pick it up and work with anyone to get this in.
@rjodinchr, Gowtham from Samsung did make progress (which is being internally reviewed) and plan to upload what we got sometime this week. Is that OK or still want to take over?
Since, we took it over and/or dont have permissions to codeplaysoftware fork, we went ahead with our own fork PR. And its here. https://github.com/google/clspv/pull/1117
This Pr (https://github.com/google/clspv/pull/925) is referenced there. So, I think we can close this PR and continue any discussions on the new one.