[AMDGPU] Treat printf as builtin for OpenCL

[vikramRH]

This is a prerequisite to enable opencl hostcall printf. ensures that AMDGPU printf calls are lowered at clang CodeGen for both HIP and OCL.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 5353d3f509814a44093a61c2725fdfe7273aa25a 9833353ab6d7bb9716883b89f4e8b90285c1a60c -- clang/lib/AST/Decl.cpp clang/lib/Basic/Targets/AMDGPU.cpp clang/lib/CodeGen/CGBuiltin.cpp

View the diff from clang-format here.

diff --git a/clang/lib/Basic/Targets/AMDGPU.cpp b/clang/lib/Basic/Targets/AMDGPU.cpp
index 0cf6daee87..307cfa49f5 100644
--- a/clang/lib/Basic/Targets/AMDGPU.cpp
+++ b/clang/lib/Basic/Targets/AMDGPU.cpp
@@ -92,8 +92,7 @@ static constexpr Builtin::Info BuiltinInfo[] = {
 #define TARGET_BUILTIN(ID, TYPE, ATTRS, FEATURE)                               \
   {#ID, TYPE, ATTRS, FEATURE, HeaderDesc::NO_HEADER, ALL_LANGUAGES},
 #define LANGBUILTIN(ID, TYPE, ATTRS, LANG)                                     \
-  { #ID, TYPE, ATTRS, nullptr, HeaderDesc::NO_HEADER, LANG }                   \
-  ,
+  {#ID, TYPE, ATTRS, nullptr, HeaderDesc::NO_HEADER, LANG},
 #include "clang/Basic/BuiltinsAMDGPU.def"
 };
 

[jhuber6]

Any tests? Can you explain why it's not sufficient to do this lowering in the AMDGPU pass?

[jhuber6]

I'm very close to landing 'real' printf support in the GPU libc where printf is just a regular function call. Will this change the handling for that in any way? I've already had to make the backend pass respect -fno-builtins and remove ockl from OpenMP to make that possible so I'm hoping we don't end up with a lot more special casing for printf.

[b-sumner]

@jhuber6 I do not believe there are any current plans to use GPU libc from HIP or OpenCL. So there will continue to be a division between OpenMP and HIP and OpenCL printf handling.

[jhuber6]

If we do the eager replacement of printf that HIP and OpenCL uses currently then it won't be linked in. So users should still be able to link in stuff like strcmp or whatever without it interfering. This would require the new driver however, and if they attempted to use something like fputs it would segfault because no one initialized the buffer, which isn't a terrible failure mode all things considered.

[vikramRH]

@jhuber6 , I had your implementation in mind when I wrote this, The printf wont be expanded by clang with "-fno-builtin" and users would still have an option to use a lib variant if need be. This also makes code more elegant as we would not have to hack the "fno-builtin" handling into the implementation, this is part of clang builtin handling.

[arsenm]

Why do you need to define a new target builtin, just to hack it to the generic lang builtin later? Just handle the existing printf builtin?

[vikramRH]

This is specifically to recognize the OpenCL version of printf (where fmt string arg is a pointer to const address space) as a builtin. The hack to generic builtin is just a option that I had as I did not want to add a new case to builtin expansion code (since the API used by both OpenCL and HIP are same ), however Im okay with adding a new case too if you feel it makes more sense.

[ssahasra]

I still don't see why this is necessary. A target-defined language-specific builtin is a whole new beast. What is missing in the current parsing of OpenCL printf?

[vikramRH]

@ssahasra , I still feel this is the way to move here since I dont see a way to access the printf option at IR level (i.e during optimization pipeline) and thus decide version of printf to use. It has to be at clang CodeGen. I ask other reviewers too if they feel there are major concerns with adding such a builtin variant (i.e AMDGPU and OCL specific). I might have to look for alternative approaches if so.

[arsenm]

If you're handling the builtin in clang directly, you can go off the original Builtin::BIprintf. I don't see what the alias AMDGPU::BIprintf is doing.

[vikramRH]

OpenCL spec says printf format string should be in constant address space. This makes the OCL printf signature target specific and hence we would need a target specific builtinID to recognize this. Im not sure I understand how we can go ahead with generic "BIPrinf" here ?

[arsenm]

The address space makes the type language dependent, it does not make it target dependent

[ssahasra]

I think what @vikramRH is saying is that the magic number "4" for OpenCL address space "__constant" is specific to AMDGPU.

[vikramRH]

Any tests? Can you explain why it's not sufficient to do this lowering in the AMDGPU pass?

I intended these changes to be part of #72556, but it seemed too many changes at one place, so I extracted this part out for ease of review. This cannot be merged standalone and has to be with 72556 , the tests are also part of that patch (This really should have been a stack of patches :( ).

Also for AMDGPU pass, I plan to remove that altogether and handle all printf lowering at one place during clang Codegen. since we now use a compiler option to switch between different implementations, This makes a lot more sense I feel.

[arsenm]

Is this redundant with #68515? Do we just need to add OpenCL test coverage?

[vikramRH]

closing this in favour of #86801