This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Respect `nobuiltin` when converting `printf`
ClosedPublic

Authored by jhuber6 on Aug 21 2023, 7:31 PM.

Details

Summary

The AMDGPU backend uses a pass to transform calls to the printf
function to a built-in verision for either HIP or OpenCL. Currently this
does not respect -fno-builtin and is always emitted. This allows the
user to turn off this functionality as is standard for these types of
built-in transformations. The motivation behind this change is to allow
the libc project to provide a linkable version of the printf
function in the future.

Diff Detail

Event Timeline

jhuber6 created this revision.Aug 21 2023, 7:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2023, 7:31 PM
jhuber6 requested review of this revision.Aug 21 2023, 7:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2023, 7:31 PM
sameerds added inline comments.Aug 21 2023, 11:14 PM
llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp
441

Attribute::Builtin is documented as "only valid on a callsite", so there shouldn't be any need to check it here?

llvm/test/CodeGen/AMDGPU/printf_nobuiltin.ll
19

There should be another test where "-fno-builtin" is passed or equivalently, printf is declared with nobuiltin, but then an over-riding "builtin" is specified on a call to printf.

jhuber6 updated this revision to Diff 552313.Aug 22 2023, 4:37 AM

Changing logic for nobuiltin and builtin

sameerds added inline comments.Aug 22 2023, 6:33 AM
llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp
443–445

But doesn't (!CI->isNoBuiltin()) cover all of this? It calls hasFnAttrImpl(), which in turn checks both attributes in the positive and negative sense, including on the called function. The overall effect is as the function comment says: "check whether this call should not be treated as a builtin".

jhuber6 added inline comments.Aug 22 2023, 7:00 AM
llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp
443–445

You're right, totally forgot that it checks the callee attributes as well.

jhuber6 updated this revision to Diff 552343.Aug 22 2023, 7:01 AM

Remove redundant check

sameerds accepted this revision.Aug 22 2023, 8:52 AM
This revision is now accepted and ready to land.Aug 22 2023, 8:52 AM
This revision was landed with ongoing or failed builds.Aug 22 2023, 10:48 AM
This revision was automatically updated to reflect the committed changes.