[PATCH] D71365: expand printf when compiling HIP to AMDGPU

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds marked 2 inline comments as done.
sameerds added inline comments.


================
Comment at: clang/test/CodeGenHIP/printf.cpp:18
+}
+
+// CHECK: [[BEGIN:%.*]]   = call i64 @__ockl_printf_begin(i64 0)
----------------
arsenm wrote:
> sameerds wrote:
> > sameerds wrote:
> > > arsenm wrote:
> > > > 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?
> > > 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.
> > Bump!
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71365/new/

https://reviews.llvm.org/D71365