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

[WIP][AMDGPU] Enable hostcall printf for OpenCL #70932

Closed
wants to merge 1 commit into from

Conversation

vikramRH
Copy link
Contributor

@vikramRH vikramRH commented Nov 1, 2023

Attempt to enable OpenCL hostcall printf (SWDEV-204804). I would like to have some inputs regarding few key points listed here,

  1. We continue to use "-mprintf-kind" option to decide the lowering scheme. It now supports a new value "none" that just makes compiler use default lowering scheme. (hostcalls for HIP, Buffered for OpenCL)
  2. OpenCL now treats printf as a builtin so that the printf expansion happens via clang CodeGen. This would also mean that the "AMDGPUPrintfRuntimeBinding" pass would no longer be required since all printf calls would be expanded earlier.
  3. The implementation adds vector processing support both for hostcall and buffered cases. The vector elements are extracted and pushed onto the buffer individually (each alingned to 8 byte boundary)
  4. for OpenCL hostcall case, The format string pointer is addrspace casted to generic address space to be compatible with hostcall device lib functions.

@arsenm
Copy link
Contributor

arsenm commented Nov 1, 2023

For point 1, I would prefer to decouple the printf implementation choice from the language. I would expect consistent defaults regardless of the language. This also contradicts point 2? I thought the clang emitted path used hostcall, and the backend path did not.

As for expanding the printf implementation capabilities to cover vectors, that should be a separate patch from any behavior change

@vikramRH
Copy link
Contributor Author

vikramRH commented Nov 2, 2023

@arsenm, The choice of defaults is based on current state of printf. HIP uses hostcalls and OpenCL uses buffered variant by default. However I'm willing to make one of the two variants consistent (preferably hostcalls for me). Do note that this would make all OpenCL printf to be lowered to hostcalls from this patch onwards. @b-sumner , are you okay with this ?

Also regarding point 2, The current behaviour is that with HIP, printf is lowered at clang CodeGen (since it treats printf to be a builtin) while OpenCL uses the IR pass. This patch will make it so that we could remove the IR pass and handle all lowering at clang CodeGen

@vikramRH
Copy link
Contributor Author

@b-sumner, @ssahasra , what are your thoughts about making the hostcall default throughout ?
Also I do not think splitting this into separate patches is feasible since changes are interdependent, however I could split changes into separate commits to make it easier to review.

@b-sumner
Copy link

@vikramRH I thought the plan was that the default would switch to hostcall, but that the OpenCL runtime would override it if it detected that the target doesn't support hostcall / PCIe.

@vikramRH vikramRH closed this Nov 16, 2023
@vikramRH
Copy link
Contributor Author

Two different patch sets have been created here,

#72554
#72556

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.

None yet

3 participants