Add new hostcall implementation#8409
Conversation
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
There was a problem hiding this comment.
Pull request overview
Adds an alternative hostcall buffer implementation (phase/occupancy-based), gated behind USE_NEW_HOSTCALL_IMPL, and wires it into both ROCm and PAL virtual GPU hostcall buffer setup along with build-system toggles.
Changes:
- Introduces a new hostcall buffer layout and processing path based on per-packet device/host phase arrays plus a device-local “occupied” bitfield.
- Updates ROCm and PAL
VirtualGPU::getOrCreateHostcallBuffer()to allocate/initialize the occupied bitfield and call the newenableHostcallsoverload when enabled. - Adds a CMake option and compile definition plumbing to turn the new implementation on/off at build time.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/clr/rocclr/device/rocm/rocvirtual.cpp | Allocates and zero-initializes an occupied bitfield buffer and uses the new enableHostcalls overload under USE_NEW_HOSTCALL_IMPL. |
| projects/clr/rocclr/device/pal/palvirtual.cpp | Same hostcall occupied-bitfield setup and new enableHostcalls overload for PAL path. |
| projects/clr/rocclr/device/devhostcall.hpp | Adds new enableHostcalls overload and introduces the new hostcall buffer/header layout under USE_NEW_HOSTCALL_IMPL. |
| projects/clr/rocclr/device/devhostcall.cpp | Implements the new hostcall buffer sizing/initialization and a new listener consumption loop under USE_NEW_HOSTCALL_IMPL. |
| projects/clr/rocclr/cmake/ROCclr.cmake | Propagates USE_NEW_HOSTCALL_IMPL as a compile definition to rocclr when enabled. |
| projects/clr/CMakeLists.txt | Adds the USE_NEW_HOSTCALL_IMPL CMake option. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@chrispaquot would you mind checking this? It is the "improve hostcall performance patch" but now as an added implementation so it can be toggled on at ROCm build time. |
## Motivation Temporary switch to enable the new hostcall version before fully commiting it ## Technical Details Adds a new THEROCK cmake variable and propagates it to the submodules using it. ## Test Plan Build with variable set to OFF and ON ## Test Result Verified correct build operation ## Submission Checklist - [x ] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests. Pre-req: ROCm/llvm-project#3311 and ROCm/rocm-systems#8409
|
TheRock and device libs patches have landed. Could I get a review for this one too? |
gandryey
left a comment
gandryey
left a comment
There was a problem hiding this comment.
Findings
Finding 1 — fillBuffer synchrony unverified (palvirtual.cpp:4027, rocvirtual.cpp:5288)
blitMgr().fillBuffer() to zero occupiedBuf may be GPU-async. If the zero-fill hasn't completed before enableHostcalls returns, the first kernel sees a garbage occupied bitfield — silent corruption. Please add a comment confirming this is synchronous, or add an explicit flush/wait.
Finding 2 — Drain scan fires unconditionally under zero load (devhostcall.cpp:440–448)
After the spin loop, resetScanLimit + full processPackets under listenerLock always runs — even when the spin found no work at all. In the idle case this fires a full num_packets-wide scan under lock every 1 ms for nothing. Add a was_busy flag; only drain when the spin actually found work.
Finding 3 — Dead nullptr check after new (devhostcall.cpp:617)
hostcallListener = new HostcallListener();
if (!hostcallListener) { ... } // unreachable: plain new throws, never returns null
Remove the check or switch to new (std::nothrow) if a non-throwing path is intended.
Finding 4 — Old PAL path leaks hostcallBuffer_ on enableHostcalls failure (palvirtual.cpp)
The new path correctly calls svmFree(hostcallBuffer_) on failure. The old #else path does not. Pre-existing bug, but now clearly visible by comparison — worth fixing here.
Finding 5 — Device-side ordering contract undocumented (devhostcall.cpp — processPackets)
The phase protocol relies on the device doing a store-release to device_phase_[i] after writing payload/header. The struct comment says "Shared prefix: must match device-side layout" but doesn't name the device-side header or assert the offsets. Please add a comment naming the contract and where the device-side definition lives.
Finding 6 — Missing EOF newline (devhostcall.hpp:239)
Motivation
This adds a new hostcall implementation, guarded by USE_NEW_HOSTCALL_IMPL
Technical Details
Adds a new implementation based on the approach developed by Joseph Huber.
JIRA ID
N/A
Test Plan
Run any programs using hostcall and verify correct execution. Run benchmarks to verify improved performance
Test Result
All runs successful.
Submission Checklist