Hostcall is a service that allows a kernel to submit requests to the
host using shared buffers, and block until a response is
received. This will eventually replace the shared buffer currently
used for printf, and repurposes the same hidden kernel argument. This
change introduces a new ValueKind in the HSA metadata to represent the
hostcall buffer.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp | ||
---|---|---|
862–863 | Is this metadata really necessary if the runtime can just check if the symbol is used? |
llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp | ||
---|---|---|
862–863 | The metadata itself is necessary because the runtime is not aware of the sequence of hidden arguments. It depends on the metadata to identify each of them. |
llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp | ||
---|---|---|
424 | I would reorder these to try hostcall first, but it's not important | |
426 | This should probably be using the LLVMContext error, instead of the MCContext error to at least report the function context in the error, e.g: DiagnosticInfoUnsupported NoGraphicsHSA( Fn, "unsupported non-compute shaders with HSA", DL.getDebugLoc()); DAG.getContext()->diagnose(NoGraphicsHSA); There should also be a testcase hitting this error |
llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp | ||
---|---|---|
426 | Moved this whole check to the printf runtime binding pass. This is more appropriate since the check is happening where the printf feature is actually processed. There already was a test for the error check, replaced it with a newer test. |
llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp | ||
---|---|---|
426 | Why is MIR testing relevant here? printf and hostcall are high-level language features that we are grudgingly handling in the backend for now. Using them both in the same low-level module is undefined, and there is no reason to even write an MIR based test for them. Also, if asserting is bad, are you asking to keep the error or only remove the assert? |
llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp | ||
---|---|---|
587 | I don't really see how we could possibly see an invoke for __ockl_hostcall_internal(). If possible, we would probably mark it as nothrow or something in the library sources. I don't have much of a preference either way, but it seems cleaner to keep the CallInst precisely to document the fact that we don't expect to see an invoke here, and also to match the nearby check for calls to printf itself. |
llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp | ||
---|---|---|
426 | Because the property enforced by this pass might not be enforced in a MIR run. Asserting on a not-bug edge case MIR sometimes wastes time debugging | |
llvm/test/CodeGen/AMDGPU/opencl-printf-no-hostcall.ll | ||
17 | Drop the argument names here which I'm surprised even parse? |
llvm/test/CodeGen/AMDGPU/opencl-printf-no-hostcall.ll | ||
---|---|---|
17 | Fixed this in the commit. |
I would reorder these to try hostcall first, but it's not important