-
Notifications
You must be signed in to change notification settings - Fork 112
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
An extension that relaxes the constant address space requirement of printf format strings and string args #807
base: main
Are you sure you want to change the base?
Conversation
Suggested by Anastasia Stulova / Arm.
Thanks @pjaaskel. Are you planning or aware of any plans for an accompanying SPIR-V extension (OpenCL extended instruction set) to relax the address space requirement in SPIR-V? |
If I am not mistaken this would be the accompanying SPIRV PR: |
I somehow missed this. Exactly what I was wondering - thanks! |
---- | ||
int printf(global char * restrict format, ...) | ||
int printf(local char * restrict format, ...) | ||
int printf(private char * restrict format, ...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should have the const
qualifier, i.e.:
int printf(global const char * restrict format, ...)
int printf(local const char * restrict format, ...)
int printf(private const char * restrict format, ...)
As a side-note, the existing constant address-space overload is as follows:
int printf(constant char * restrict format, ...)
It would be good to add an overload for constant const
:
int printf(constant const char * restrict format, ...)
Otherwise, adding the const
qualifier to a string in the constant
address space may produce a warning, e.g.:
warning: passing argument 1 of 'printf' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
note: expected 'constant char *' but argument is of type 'constant const char *'.
(In my own opinion, as this arguably doesn't affect semantics, we could make this change as a fix to the OpenCL spec, rather than adding an extra overload in this extension. If others do not share that opinion, e.g. if there is a stronger effect on semantics than I have understood, then it would seem better to add an extra overload here than to leave this unchanged, though.)
Mentioned in the September 27th teleconference: The related Intel extension is here: The main difference between this extension and the Intel extension is that this extension explicitly requires support for format strings and strings passed as %s arguments that are dynamic, whereas the Intel extension explicitly requires that these strings remain string literals. I think we could support the dynamic string behavior, but it would require changes to our current implementation. |
int printf(global char * restrict format, ...) | ||
int printf(local char * restrict format, ...) | ||
int printf(private char * restrict format, ...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we support the generic address space as well if the generic address space is supported ("OpenCL C 2.0 or OpenCL C 3.0 with the __opencl_c_generic_address_space feature")?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe OpenCL puts string literals to the 'constant' AS so it would not add anything in practice, if I'm not mistaken? In the proposed extension I just clarified what are the implications of allowing these to originate from other address spaces. |
There's an example CPU implementation for the 1.2 address spaces in a PoCL branch. Looking for feedback and another implementer.
The key motivation for this extension is to minimize the need for modifications of codes using printf when moving between host and device, and to support running CUDA/HIP-based kernels which call a more relaxed printf (that in fact delegates from the device to host printf, whatever that happens to be).
@AnastasiaStulova @bashbaug @Kerilk