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

An extension that relaxes the constant address space requirement of printf format strings and string args #807

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pjaaskel
Copy link
Contributor

@pjaaskel pjaaskel commented Jun 21, 2022

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

@CLAassistant
Copy link

CLAassistant commented Jun 21, 2022

CLA assistant check
All committers have signed the CLA.

@mkinsner
Copy link

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?

@Kerilk
Copy link
Contributor

Kerilk commented Jul 12, 2022

If I am not mistaken this would be the accompanying SPIRV PR:
KhronosGroup/SPIRV-Registry#148

@mkinsner
Copy link

If I am not mistaken this would be the accompanying SPIRV PR:
KhronosGroup/SPIRV-Registry#148

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, ...)
Copy link
Contributor

@StuartDBrady StuartDBrady Sep 27, 2022

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.)

@bashbaug
Copy link
Contributor

Mentioned in the September 27th teleconference:

The related Intel extension is here:
https://github.com/intel/llvm/blob/sycl/sycl/doc/design/spirv-extensions/SPV_INTEL_non_constant_addrspace_printf.asciidoc

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.

Comment on lines +75 to +77
int printf(global char * restrict format, ...)
int printf(local char * restrict format, ...)
int printf(private char * restrict format, ...)
Copy link
Contributor

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")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pjaaskel
Copy link
Contributor Author

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 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants