hipGraph API fixes & "native" Graph implementation #560
Conversation
this is required for cl_khr_command_buffer
CL/opencl.h: include cl_ext_pocl.h but exclude cl_gl_ext.h (now just redirects to cl_ext.h)
This class is used to implement Graphs that execute "natively" in the backend, using OpenCL command-buffers or LevelZero command-lists and only synchronizing with the host when required. Fallback to original Graph is provided in chipstar::GraphExec::launch() Add more error-checking to the hipGraphXYZ APIs & bugfixes Additionally, samples/graph + samples/graphMatrixMultiply work using the "native graphs" (cl_command_buffer), not the original chip-spv's graph execution. Merged style fixes by Henry. Co-authored-by: Henry Linjamäki <[email protected]>
move graph tests to FAILING_FOR_ALL
| *pGraph = Graph; | ||
| RETURN(hipSuccess); | ||
| CHIP_CATCH | ||
| } | ||
|
|
||
| hipError_t hipGraphDestroy(hipGraph_t graph) { | ||
| CHIP_TRY | ||
| if (!graph) |
There was a problem hiding this comment.
Should start with a capital per the LLVM C++ naming style, right? There are also others in the file.
There was a problem hiding this comment.
This follows the original AMD's HIP API naming & style, see e.g. here. Not sure if it's a good idea to change it
There was a problem hiding this comment.
We already did when we decided to use the LLVM C++ style in chipStar codes.
|
|
||
| cl_command_queue CQ = ClQueue_->get(); | ||
| int err = CL_SUCCESS; | ||
| cl_command_buffer_khr Res = |
There was a problem hiding this comment.
Add a check somewhere that we are targeting only one device, if using the single-device CB.
| @@ -1291,6 +1366,401 @@ std::shared_ptr<chipstar::Event> CHIPQueueOpenCL::enqueueBarrierImpl( | |||
| return Event; | |||
| } | |||
|
|
|||
| /********************************************************************************/ | |||
|
|
|||
| chipstar::GraphNative *CHIPQueueOpenCL::createNativeGraph() { | |||
There was a problem hiding this comment.
Why GraphNative, why not NativeGraph? Or BackendGraph?
There was a problem hiding this comment.
because that's how the rest of the graph-related classes are named: GraphExec, GraphNodeMemcpy, GraphNodeMemset etc.
|
One test fails here with iGPU (both LZ and CL) and Intel CPU: hipSimpleGraphWithKernel: /home/pjaaskel/src/chip-spv/src/SPIRVFuncInfo.cc:183: void SPVFuncInfo::visitKernelArgsImpl(const std::vector<void *> &, KernelArgVisitor) const: Assertion `ArgData && "nullptr in the argument list"' failed.
Filters: Unit_hipGraph_SimpleGraphWithKernelPoCL-CPU (latest main) fails a lot of tests, but I suppose it's expected before the SVM/CB updates land to main, right? |
It works for me on my iGPU (with both LZ and CL), so i'm going to need more information to debug (llvm/translator exact commits, cmake build flags etc).
No, this PR is not dependent on PoCL updates. The native graphs are entirely optional, and the PR also passes tests with Level0 backend (which does not yet have the "native" graph implemented).
Is the 1384 from running ctest directly with no arguments ? The official runner is |
the constructors were copying the value of Args pointer, instead of making an actual copy of the arguments. This can lead to a crash if the Args pointer lives on the stack, later goes out of scope, and setupAllArgs() is called.
|
Please also add some text to ReleaseNotes of this feature. |
|
@franz conflicts |
This is the same as PR #434, except: