Page MenuHomePhabricator

support for HIP non hostcall printf
Changes PlannedPublic

Authored by vikramRH on Nov 25 2022, 3:26 AM.

Details

Summary

The patch essentially ports the existing non-hostcall printf support in OpenCL to HIP (to be controlled via new option "-fdelayed-printf"), with following changes

  1. Code refactoring -> we now use API's "getConstantStringInfo()" to extract constant string contents at compile time.
  2. Support to print non-const null terminated strings, required in HIP context. This is achieved as follows
    • calculate string size using a function "getStrlenWithNull()" and reserve the space in printf buffer using __printf_alloc() (as was the case in OpenCL, but number of bytes allocated could be dynamic now)
    • copy the string contents to buffer using previously calculated size and the pointer to string (a memcpy intrinsic is generated here)

Diff Detail

Unit TestsFailed

TimeTest
60,040 msx64 debian > libFuzzer.libFuzzer::minimize_crash.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/NullDerefTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/minimize_crash.test.tmp-NullDerefTest

Event Timeline

vikramRH created this revision.Nov 25 2022, 3:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2022, 3:26 AM
vikramRH requested review of this revision.Nov 25 2022, 3:26 AM
vikramRH planned changes to this revision.Nov 27 2022, 8:38 PM
arsenm requested changes to this revision.Nov 28 2022, 7:21 AM

I have a few questions. First, why surface this to users? If we really need to, I don't think this is the right flag name/design. A named argument to some kind of printf lowering flag would be better. Second, I thought we were moving towards hostcall printf, not away from it.

clang/include/clang/Basic/LangOptions.def
275

Typo 'onf'

clang/include/clang/Driver/Options.td
985

Help text reads weird

clang/lib/Driver/ToolChains/Clang.cpp
4655–4667

Missing clang side tests

llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp
135–136

Why not share these? They should be in the same place. This should also be a separate change from any flag changes

141

auto *

150

Why would the block be missing a terminator here?

183–185

Really should try hard to avoid introducing ptrtoint. Do you really need to do a pointer subtract?

215

Don't need llvm::

439

Broken for opaque pointers?

443

Why isn't this using the IRBuilder?

491–492

Why do we have raw new and strcpy here? We have better string classes

llvm/test/CodeGen/AMDGPU/hip-delayed-printf.ll
1

Don't need these separate prefixes

3

Should use generated checks

39

Use named values in tests

There were some issues reported where it was found that pcie atomics are not available in some environments, as a result hostcall and HIP-Printf were also not available. This Patch is intended to provide an alternative in such scenarios while hostcall based implementation remains the primary option.

After discussions with Sameer, I am convinced that it is better to do this transformation at clang CodeGen phase rather than a opt pass (similar to current hostcall based implementation),
Hence this patch needs rework and I shall include the review comments in the updated patch. (I agree on the option too, I shall think of a option name with a named argument)