Add HSA-based launcher - #81
Conversation
Introduce AMD_TRITON_NPU_RUNTIME=hsa, which dispatches Triton-generated NPU kernels through ROCR's AIE agent path instead of XRT, reusing the shared MLIR -> AIR -> PDI compile pipeline with a swapped-in HSA C++ launcher (hsa_driver.py). Shared launcher codegen helpers (type mapping, signature extraction, actual-size padding) are factored out of driver.py into _codegen.py so both drivers depend on a common surface. Under HSA the output format is forced to "pdi", and setup.py now copies all backend Python modules instead of an explicit compiler.py/driver.py allowlist.
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
clang-format
[clang-format] reported by reviewdog 🐶
[clang-format] reported by reviewdog 🐶
[clang-format] reported by reviewdog 🐶
[clang-format] reported by reviewdog 🐶
Triton-XDNA/amd_triton_npu/backend/include/HsaRuntime/HsaRuntime.cpp
Lines 494 to 496 in 26a5bed
[clang-format] reported by reviewdog 🐶
[clang-format] reported by reviewdog 🐶
[clang-format] reported by reviewdog 🐶
[clang-format] reported by reviewdog 🐶
[clang-format] reported by reviewdog 🐶
[clang-format] reported by reviewdog 🐶
Triton-XDNA/amd_triton_npu/backend/include/npu_dispatch_common.h
Lines 14 to 15 in 26a5bed
[clang-format] reported by reviewdog 🐶
Triton-XDNA/amd_triton_npu/backend/include/npu_dispatch_common.h
Lines 49 to 50 in 26a5bed
[clang-format] reported by reviewdog 🐶
Triton-XDNA/amd_triton_npu/backend/include/npu_dispatch_common.h
Lines 58 to 59 in 26a5bed
[clang-format] reported by reviewdog 🐶
Triton-XDNA/amd_triton_npu/backend/include/npu_dispatch_common.h
Lines 110 to 111 in 26a5bed
[clang-format] reported by reviewdog 🐶
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds an HSA/ROCR-based launch runtime as an alternative to XRT for the AMD NPU Triton backend, including a shared HSA runtime library and a per-signature HSA launcher path, plus configuration/docs updates to select the runtime.
Changes:
- Add a shared HSA runtime (
libtriton_npu_hsa.so) and an HSA launcher generator that dispatches via the ROCR AIE agent path. - Introduce runtime selection (
xrtvshsa) viaNPUDriver(...)andAMD_TRITON_NPU_RUNTIME, and forcepdioutput when using HSA. - Refactor common launcher codegen helpers and update packaging/docs accordingly.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.py | Copy backend Python modules more generically into installed Triton backend. |
| README.md | Document runtime selection (XRT vs HSA) and HSA requirements/usage. |
| amd_triton_npu/backend/include/npu_dispatch_common.h | New shared CPython glue helpers used by both XRT and HSA launchers. |
| amd_triton_npu/backend/include/HsaRuntime/HsaRuntime.h | New C ABI for a shared HSA/ROCR dispatch runtime. |
| amd_triton_npu/backend/include/HsaRuntime/HsaRuntime.cpp | New shared HSA runtime implementation (queue, signal, vmem pooling, program cache). |
| amd_triton_npu/backend/hsa_launcher.py | New per-signature CPython launcher generator for the HSA runtime. |
| amd_triton_npu/backend/driver.py | Add runtime selection, build/link profile handling, and shared codegen refactors. |
| amd_triton_npu/backend/config.py | Add AMD_TRITON_NPU_RUNTIME config plumbing and validation. |
| amd_triton_npu/backend/codegen.py | New shared codegen utilities for launchers (types/signature/actual-sizes). |
Comments suppressed due to low confidence (4)
amd_triton_npu/backend/include/npu_dispatch_common.h:60
- getPointer() sets a TypeError for unsupported pointer arguments but returns with ptr_info.valid still true, so callers may continue as if resolution succeeded.
PyErr_SetString(PyExc_TypeError,
"Pointer argument must be either uint64 or have data_ptr method");
return ptr_info;
amd_triton_npu/backend/include/npu_dispatch_common.h:56
- getPointer() does not handle PyLong_AsUnsignedLongLong(ret) failures from data_ptr() (e.g. negative return value). That can leave a Python error set while returning ptr_info.valid=true.
ptr_info.dev_ptr = reinterpret_cast<void *>(PyLong_AsUnsignedLongLong(ret));
Py_DECREF(ret);
return ptr_info;
amd_triton_npu/backend/include/npu_dispatch_common.h:91
- getNumElements() uses PyErr_Print() in loop error paths, which clears the underlying exception and can lead to returning NULL without an active Python exception.
PyObject *dim_obj = PySequence_GetItem(shape, i);
if (!dim_obj) {
Py_DECREF(shape);
PyErr_Print();
return -1;
amd_triton_npu/backend/hsa_launcher.py:167
- The generated C++ launcher calls launch_exit_hook but does not Py_DECREF the return value on success, leaking a reference per dispatch.
if (launch_exit_hook != Py_None) {{
PyObject* hook_args = Py_BuildValue("(O)", launch_metadata);
PyObject* ret = PyObject_CallObject(launch_exit_hook, hook_args);
Py_DECREF(hook_args);
if (!ret) return NULL;
}}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
clang-format
[clang-format] reported by reviewdog 🐶
[clang-format] reported by reviewdog 🐶
Triton-XDNA/amd_triton_npu/backend/include/npu_dispatch_common.h
Lines 14 to 15 in acdd93b
[clang-format] reported by reviewdog 🐶
Triton-XDNA/amd_triton_npu/backend/include/npu_dispatch_common.h
Lines 49 to 50 in acdd93b
[clang-format] reported by reviewdog 🐶
Triton-XDNA/amd_triton_npu/backend/include/npu_dispatch_common.h
Lines 58 to 59 in acdd93b
[clang-format] reported by reviewdog 🐶
Triton-XDNA/amd_triton_npu/backend/include/npu_dispatch_common.h
Lines 110 to 111 in acdd93b
[clang-format] reported by reviewdog 🐶
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
I'm assuming you can just use What is the Windows equivalent? (rocr is supported on Windows). Has IRON been checked for a similar situation where it may benefit from the hsa based launcher? |
|
Thank you for your review.
Yes, I'll add support for the wheels. I'll also clarify it in the instructions; people won't have to deal with building ROCR.
ROCR will need NPU driver integration for Windows. xdna-driver is Linux-only.
This is an XRT integration inefficiency, not necessarily fundamental, although ROCR shows different performance characteristics (more on this at a later time). If XRT integration is similar to ROCR's, then performance should be identical. |
erwei-xilinx
left a comment
There was a problem hiding this comment.
LGTM! Thanks for working on this!
vmem_alloc acquired a handle, a VA reservation, a mapping and an access grant with no cleanup if a later step failed, so a failure part-way through stranded a mapping. That is worse than an ordinary leak: a stranded mapping makes the next allocation that reserves the same virtual address fail, so the damage lands on unrelated allocations later rather than on the call that failed. Undo each step if a subsequent one throws, reporting the reclaim statuses through log_status. The access-grant failure path unmaps directly instead of revoking first, since no grant was ever applied. Found in Xilinx/mlir-aie#3452, which hit the resulting allocate -> free -> allocate failure on its Python HSA runtime. The failure does not reproduce here (the allocator picks a fresh VA rather than colliding), so this is verified only as a leak fix: fault injection confirms the unwind runs and that unmap, address_free and handle_release all succeed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Under the HSA runtime the backend still shelled out to xrt-smi purely
to tell npu1 from npu2, so an HSA-only host needed XRT installed just
to identify the device. Ask the AIE agent instead.
Expose triton_npu_hsa_agent_name() from the shared runtime and call it
from detect_npu_version(), which now takes the caller's runtime
("xrt"/"hsa"); _get_output_format and compile_module pass the one they
already hold. The query reuses libtriton_npu_hsa.so rather than
dlopen-ing ROCR separately from Python, keeping one hsa_init and one
HsaRuntime per process, which the agent requires at QUEUES_MAX == 1.
The name comes from HSA_AGENT_INFO_NAME ("aie2" on npu1, "aie2p" on
npu2), not the agent's ISA: ROCR reports no ISA for the AIE agent here
(the query fails and leaves a null handle).
An agent that answers with an unrecognized name raises
UnsupportedNPUDeviceError rather than falling back to xrt-smi -- a
second opinion would only supply a generation for hardware this
backend has never been validated against, and compiling for the wrong
one wedges the device until the driver timeout fires. A failed *query*
(no ROCR, no agent) still falls back, since it says nothing about the
hardware. AMD_TRITON_NPU_TARGET continues to override both.
Idea from Xilinx/mlir-aie#3452.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The unwind added in b8f814c let HSA_CHECK throw, caught it purely to release what had been acquired, and rethrew. Check each status directly instead: on failure, run the reclaim steps and then throw once. Add hsa_error(), which builds the runtime_error rather than throwing it, so a caller that must clean up first can unwind and throw at the end. HSA_CHECK now uses it too, leaving one implementation of the message format instead of two. Name the individual reclaim steps release_address() and release_handle(), shared with vmem_free so each has a single definition. They report through log_status rather than throwing, which is what keeps the single-throw property: an allocation failure surfaces its own status, not one raised by the cleanup. Error messages improve as a side effect. HSA_CHECK stringifies the expression, so the old path reported "set_vmem_access(b.va, size, true) failed: ..."; it now names the underlying call, "hsa_amd_vmem_set_access failed: ...". Fault injection at the access-grant step confirms the deepest unwind path still runs, that unmap, address_free and handle_release all succeed, and that later dispatches are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
No description provided.