This is an archive of the discontinued LLVM Phabricator instance.

expand printf when compiling HIP to AMDGPU
ClosedPublic

Authored by sameerds on Dec 11 2019, 9:56 AM.

Details

Summary

This change implements the expansion in two parts:

  • Add a utility function emitAMDGPUPrintfCall() in LLVM.
  • Invoke the above function from Clang CodeGen, when processing a HIP program for the AMDGPU target.

The printf expansion has undefined behaviour if the format string is
not a compile-time constant. As a sufficient condition, the HIP
ToolChain now emits -Werror=format-nonliteral.

Event Timeline

sameerds created this revision.Dec 11 2019, 9:56 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 11 2019, 9:56 AM
sameerds edited the summary of this revision. (Show Details)Dec 11 2019, 9:57 AM
sameerds added reviewers: arsenm, b-sumner.
sameerds added a reviewer: yaxunl.
sameerds updated this revision to Diff 236312.Jan 6 2020, 1:58 AM
sameerds edited the summary of this revision. (Show Details)

Improved the test defined in clang/test/CodeGenHIP/printf.cpp

arsenm added inline comments.Jan 6 2020, 7:51 AM
clang/test/CodeGenHIP/printf.cpp
18

This could use a lot more testcases. Can you add some half, float, and double as well as pointers (including different address spaces) and vectors?

sameerds added inline comments.Jan 6 2020, 9:19 PM
clang/test/CodeGenHIP/printf.cpp
18

I am not sure what exactly should be tested. The validity of this expansion depends on the signature of the builtin printf function. Since printf is variadic, the "default argument promotions" in the C/C++ spec guarantee that the arguments are 32/64 bit integers or doubles if they are not pointers. The printf signature as well as the device library functions are defined using only generic pointers, so the address space does not matter. Non-scalar arguments are not supported, which is checked by another test using a struct. I could add a vector there, but that doesn't seem to be adding any value.

Should this be looking forward to also handling OpenCL, which does require vector support?

Should this be looking forward to also handling OpenCL, which does require vector support?

Sure. The implementation is general enough that we can point at the two places in AMDGPUEmitPrintf.cpp that need to be enhanced for OpenCL:

  1. processArg() should recognize a vector argument and transmit its elements to the host correctly.
  2. locateCStrings() should deal with all the additional OpenCL specifiers (like vectors and half data-types) as needed.

Then depending on the input language, Clang codegen can indicate whether to support these extensions or emit an error.

sameerds added inline comments.Jan 9 2020, 8:32 PM
clang/test/CodeGenHIP/printf.cpp
18

Bump!

arsenm added inline comments.Jan 13 2020, 9:44 AM
clang/test/CodeGenHIP/printf.cpp
18

The address space shouldn't matter, but it's good to make sure it doesn't.

Vector arguments are 100% supported in OpenCL printf, and are not subject to argument promotion. You have to use a width modifier for them though.

Mostly looks fine, except vectors are supposed to work

llvm/lib/Transforms/Utils/AMDGPUEmitPrintf.cpp
52

No else after return

57–58

Dead code

sameerds updated this revision to Diff 237864.Jan 14 2020, 12:15 AM
  • Added a test for address spaces
  • Removed an unnecessary addrspacecast in the strlen expansion
  • Updated the tests to pass -fcuda-is-device
  • printf is not a builtin for device code, so removed the code and tests related to that
sameerds marked 2 inline comments as done.Jan 14 2020, 12:20 AM
sameerds added inline comments.
clang/test/CodeGenHIP/printf.cpp
18

Added a test with address spaces on the pointer arguments.

Vectors are supported in OpenCL, but this initial implementation is specifically for HIP. The printf expansion is invoked by Clang CodeGen only when the language is HIP.

Vector support will arrive later. It's not sufficient to just implement the transmission via hostcall; it also needs changes to the printf processing performed by the hostcall listener thread on the host.

arsenm accepted this revision.Jan 15 2020, 6:04 AM

LGTM

This revision is now accepted and ready to land.Jan 15 2020, 6:04 AM
This revision was automatically updated to reflect the committed changes.