Skip to content

Add ROCm 10 backend for AMD GPUs - #7

Open
mehmetoguzderin wants to merge 5 commits into
deepseek-ai:mainfrom
mehmetoguzderin:feat/rocm-10
Open

mehmetoguzderin wants to merge 5 commits into
deepseek-ai:mainfrom
mehmetoguzderin:feat/rocm-10

Conversation

@mehmetoguzderin

Copy link
Copy Markdown

Add an opt-in deep_jit::ROCm backend for ROCm 10, following the existing backend structure.

  • Compile HIP source to HSACO with HIPCC, and load and launch kernels through HIP.
  • Reuse the existing runtime, caches and PyTorch bindings, including current-stream integration.
  • Add ROCm tests and documentation, and remove unused CUDA dependencies from the optional root module/build.

The shared runtime, caches and existing backend implementations are unchanged. CUDA and Ascend consumers keep their existing workflows without acquiring ROCm dependencies. Their header tests exclude ROCm-only headers.

Testing

Tested on AMD Instinct MI355X (288 GB, gfx950), Ubuntu 26.04 LTS, ROCm 10.0.0 (HIP 7.15.26333), PyTorch torch[device-gfx950]==2.13.0+rocm10.0.0.

Ran python tests/test_rocm.py, exercised end-to-end PyTorch integration beyond the main test suite, and checked existing backends for regressions.

Add an opt-in `deep_jit::ROCm` backend for ROCm 10, following the existing backend structure.

- Compile HIP source to HSACO with HIPCC, and load and launch kernels through HIP.
- Reuse the existing runtime, caches and PyTorch bindings, including current-stream integration.
- Add ROCm tests and documentation, and remove unused CUDA dependencies from the optional root module/build.

The shared runtime, caches and existing backend implementations are unchanged. CUDA and Ascend consumers keep their existing workflows without acquiring ROCm dependencies. Their header tests exclude ROCm-only headers.

### Testing

Tested on AMD Instinct MI355X (288 GB, `gfx950`), Ubuntu 26.04 LTS, ROCm 10.0.0 (HIP 7.15.26333), PyTorch `torch[device-gfx950]==2.13.0+rocm10.0.0`.

Ran `python tests/test_rocm.py`, exercised end-to-end PyTorch integration beyond the main test suite, and checked existing backends for regressions.
@mehmetoguzderin

Copy link
Copy Markdown
Author

Hi @guyan364, @kurisu6912, @LyricZhao, thank you very much for authoring and releasing this library, I really like its architecture. I tried to enable an AMD GPU backend on ROCm 10 and tested all the way through to Torch without regressing other backends (and removed an unused CMake import). Although I tried to stay close to the existing code conventions in the repository, please let me know if you would prefer any code changes. All the best. - Oguz

@bong-water-water-bong

Copy link
Copy Markdown

A note from use, not a review — with one correction that is mine.

I ran this backend on a Strix Halo (gfx1151, Radeon 8060S) with the ROCm SDK's aie2p clang. Build: hipcc -x hip --genco --offload-arch=gfx1151 -O3 --no-gpu-bundle-output → 4.7 KB HSACO in ~0.5 s. Load path end to end: hipLibraryLoadFromFile → GetKernelCount(1) → EnumerateKernels → hipKernelGetFunction → hipModuleLaunchKernel, with correct results on a scale kernel.

The dlsym-based use of the hipLibrary* entry points is load-bearing here, and the reason is worth recording: on this toolchain the HIP headers do not declare hipLibraryEnumerateKernels / hipKernelGetFunction (only LoadFromFile, Unload, GetKernelCount), although libamdhip64.so.7 exports all five. Code that calls them by name does not compile — so the lazy-dlsym approach is not stylistic, it is required by the runtime/header skew.

The correction, because it is the part I got wrong first: I initially read two "undeclared identifier" errors in driver.hpp as this PR failing to build. That was not this PR's defect — it was my header set. hipcc resolves <hip/hip_runtime.h> to /usr/include/hip (the distro libamdhip64-dev headers) rather than the SDK's devel include tree, and that system set lacks those two declarations; the SDK's devel tree declares all five. Passing the devel tree with -isystem (a plain -I does not win that lookup) compiles driver.hpp, device.hpp, options.hpp, kernel.hpp and backend.hpp cleanly, with no change to the PR.

So: not a defect here — but the practical consequence may be worth a line in the backend's README, since the backend effectively requires ROCm headers that match the runtime it will be loaded by, and a system HIP package that is older than the runtime produces exactly those two errors rather than a link failure. That is a diagnosability note, not a request.

Unrelated and minor, since I noticed it while reading the repo: the workflow in #8 requests pull-requests: write and security-events: write. On a repo that has not asked for third-party CI it is a broader grant than a read-only scan needs — worth a look if you have not reviewed it.

Nothing here blocks the PR from my side; the ROCm path worked on the first architecture I had.

@bong-water-water-bong

Copy link
Copy Markdown

The ROCm backend requires HIP headers that match the HIP runtime. With a header
set that predates the ROCm 10 library API, include/deep_jit/backend/rocm/driver.hpp
fails to compile with exactly two errors:

driver.hpp:19: error: use of undeclared identifier 'hipLibraryEnumerateKernels'
driver.hpp:20: error: use of undeclared identifier 'hipKernelGetFunction'

Reason, for anyone who hits it: DJ_DECL_LAZY_DL_FUNCTION ends in
reinterpret_cast<decltype(&name)>(symbol), so decltype(&name) needs name to be
declared even though the symbol is resolved at runtime with dlsym. The dlsym
indirection is therefore load-bearing for version tolerance but does not remove
the compile-time dependency on the declaration.

Concretely, on Strix Halo (gfx1151, TheRock HIP 7.16): the SDK's
hip_runtime_api.h declares all five entry points, and the backend compiles
cleanly against it (driver.hpp, device.hpp, options.hpp, kernel.hpp,
backend.hpp → 0 errors with -fsyntax-only -std=c++20 -D__HIP_PLATFORM_AMD__ -x hip). A distro HIP header set present on the same box declares only three and
produces exactly the two errors above. So the failures are a header/runtime
mismatch on the consumer side, not a defect in this backend.

Suggestion: one sentence in the ROCm section of the README — "requires HIP headers
from the same ROCm installation as the runtime; drivers error with undeclared
hipLibraryEnumerateKernels/hipKernelGetFunction when an older header set is
picked up first."

While here, two things that are verified and might be worth a line each:

  • hipcc silently prefers the distro header set: a user -I <sdk>/include does
    not win, -isystem <sdk>/include does (verified with -H on a real translation
    unit). That is the usual way to hit the error above.
  • The backend's two hipLibrary* calls are also permitted to fail at runtime:
    require_library_api() asserts on the same names, which is the right behaviour,
    so a stale runtime fails loudly rather than misbehaving.

What this note is not

It is not a bug report and not a patch. My first pass concluded the backend
did not build and drafted a two-declaration patch; that conclusion was wrong — it
was produced by compiling against the wrong header set. With the SDK headers the
unmodified PR is clean. Please disregard the earlier draft if it was relayed
anywhere; the retraction is the substance of this note.

Evidence

  • Compile harness: hipcc -fsyntax-only -std=c++20 -D__HIP_PLATFORM_AMD__ -I<DeepJIT>/include -isystem <torch>/include -isystem <pybind11>/include -isystem <python>/include -isystem <_rocm_sdk_devel>/include -x hip <each header>
    → 0 errors on all five headers, PR head 2a47e15.
  • Header inventory: <_rocm_sdk_devel>/include/hip/hip_runtime_api.h declares
    hipLibraryLoadFromFile, hipLibraryUnload, hipLibraryGetKernelCount,
    hipLibraryEnumerateKernels, hipKernelGetFunction; /usr/include/hip/hip_runtime_api.h
    declares the first three only.
  • Runtime check: libamdhip64.so.7 exports all five (nm -D), so the symbols
    exist even where the headers lag.
  • Load/launch path exercised end to end on gfx1151: hipLibraryLoadFromFile →
    hipLibraryGetKernelCount (1) → hipLibraryEnumerateKernels → hipKernelGetFunction
    → hipModuleLaunchKernel, correct result.
  • Checked 2026-09-11.

Clarify HIP header requirements and library compatibility checks for ROCm 10.
@mehmetoguzderin

Copy link
Copy Markdown
Author

@bong-water-water-bong Thank you very much for the check, I agree that it helps to note such aspects to ease user on-boarding. I added a small paragraph aligning with your suggestion, could you please check (just in case you have the time for it)? Appreciated in advance.

@bong-water-water-bong

Copy link
Copy Markdown

Checked — the paragraph is accurate as written, and I re-ran the checks rather
than repeating my earlier note, so here is the output behind those statements.

Headers. The two sets on the box disagree in exactly the way the paragraph
describes:

header set LoadFromFile Unload GetKernelCount EnumerateKernels hipKernelGetFunction
<sdk-devel>/include/hip/hip_runtime_api.h (HIP 7.16.26332) ✓ ✓ ✓ ✓ ✓
/usr/include/hip/hip_runtime_api.h (HIP 7.1.52801) ✓ ✓ ✓ – –

Compile, PR head 240ffc4, five headers, hipcc -fsyntax-only -std=c++20 -D__HIP_PLATFORM_AMD__ -I <DeepJIT>/include -x hip <header>:

  • with -isystem <sdk-devel>/include: driver.hpp, device.hpp, options.hpp
    → 0 errors. (kernel.hpp and backend.hpp stop at
    ATen/hip/impl/HIPStreamMasqueradingAsCUDA.h, i.e. they need the headers from a
    ROCm PyTorch build; this box only has a CPU torch today. That is unrelated to
    this PR — the same include is what makes kernel.hpp uncompilable here, not the
    library API.)
  • without it: driver.hpp:19 and driver.hpp:20 → the two undeclared
    identifiers, each with the follow-on reference to overloaded function could not be resolved, and every header that includes driver.hpp inherits them.

Resolution, -H on the same translation unit, which is the part worth the
line in the README:

-isystem <sdk-devel>/include  ->  <sdk-devel>/include/hip/hip_runtime.h
-I       <sdk-devel>/include  ->  /usr/include/hip/hip_runtime.h

So "plain -I may not override that lookup" is exactly right — nothing wins the
hip/ lookup against the compiler's own /usr/include search, only -isystem
reorders it ahead of the default set.

Runtime. One addition you may want in that paragraph, because it is what the
compatibility check actually sees: libamdhip64.so.7 is not one artifact. On the
same box:

libamdhip64.so.7 HIP version exports of the five require_library_api()
ROCm SDK runtime 7.16.26332 5/5 passes (silent)
distro 7.1.52801 3/5 throws, naming hipLibraryEnumerateKernels
HRX drop-in runtime (ships its own libamdhip64.so.7 over libhrx.so.0) 7.9999.0 0/5 throws, naming hipLibraryLoadFromFile

I ran require_library_api() against each with LD_LIBRARY_PATH selecting it; the
first is silent, the other two raise the documented compatibility error naming the
first missing symbol, before any library is loaded. That is the behaviour the
paragraph after yours describes, and the reason it is worth keeping: an
HRX-shaped runtime (classic HIP entry points, no library-enumeration API) fails
with a named symbol instead of an unexplained dlsym failure later.

One correction to my earlier note, since you may have read it: I wrote that
"libamdhip64.so.7 exports all five (nm -D)". That holds for the ROCm SDK
runtime; the distro 7.1 runtime exports the same three its headers declare. Both
stacks are internally consistent — the skew is only when headers and runtime come
from different installations, which is precisely the case your parenthesis points
at.


Evidence commands (all re-run 2026-09-11 on the gfx1151 box):

# header inventory
grep -c 'hipLibraryEnumerateKernels *(' <each hip_runtime_api.h>
# compile matrix + resolution
hipcc -fsyntax-only -std=c++20 -D__HIP_PLATFORM_AMD__ -I <DeepJIT>/include \
  -isystem <sdk-devel>/include -x hip <header>      # and again without -isystem
hipcc ... -H <driver.hpp>                           # -isystem vs -I
# runtime exports
nm -D --defined-only <libamdhip64.so.7> | grep -c '<symbol>@'
# guard behaviour
LD_LIBRARY_PATH=<each runtime dir> ./require_library_api_probe

@mehmetoguzderin

Copy link
Copy Markdown
Author

@bong-water-water-bong awesome, thank you. Your experience and analysis were helpful to better document this PR, and hopefully provide a good data point for maintainers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants