[PR #89] [MERGED] Improve alignment between the ROCm and CUDA backends #111

New issue

Closed

opened 2026-10-26 22:24:38 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:24:38 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-aimdo/pull/89
Author: @0xDELUXA
Created: 8/15/2026
Status: ✅ Merged
Merged: 9/7/2026
Merged by: @rattus128

Base: master ← Head: amd/rocm-parity

📝 Commits (5)

9a5da80 hip: prefer the ROCm 7 runtime and correct dispatch symbol resolution
6bef92b shmem-detect: drop vendor specific sysmem fallback wording
2614512 control: prefer torch.version.hip over the local version segment
8d50cea vrambuf: lock and tear down the rocm windows VA reservation pool
21e94b6 control: budget integrated rocm GPUs against system RAM on linux

📊 Changes

8 files changed (+88 additions, -7 deletions)

View changed files

📝 comfy_aimdo/control.py (+11 -0)
📝 src-hip/dispatch.c (+14 -3)
📝 src-win/shmem-detect.c (+1 -1)
📝 src/control.c (+5 -3)
📝 src/control.h (+2 -0)
📝 src/gpu_abi.h (+8 -0)
📝 src/plat.h (+9 -0)
📝 src/vrambuf.c (+38 -0)

📄 Description

Five places where the HIP backend has drifted from the CUDA one, found by diffing the two side by side. Some are plainly bugs, some are divergences, marked below.

Integrated GPU budgeting was CUDA only. github.com/Comfy-Org/comfy-aimdo@159abbe landed behind a !defined(__HIP_PLATFORM_AMD__) guard, so APUs on ROCm get budgeted against hipMemGetInfo rather than /proc/meminfo MemAvailable. Enabling it needs a per-backend attribute value: hipDeviceAttributeIntegrated is 16, so reusing the CUDA 18 would have queried hipDeviceAttributeKernelExecTimeout. Whether the MemAvailable heuristic suits an AMD APU needs testing.
The hipGetDeviceProperties fallback returns the wrong struct layout. shmem-detect.c reads uuid and luid, which the R0000 layout does not have, so the fallback can only ever produce a bogus LUID and a failed adapter match. Removed rather than fixed. Latent; the R0600 lookup succeeds on current runtimes.
The ROCm/Windows VA pool was unsynchronised and leaked. github.com/Comfy-Org/comfy-aimdo@2362071 added a mutex, github.com/Comfy-Org/comfy-aimdo@c760f6a moved the pool into AimdoContext and dropped it, but that struct is shared across threads and vrambuf_destroy runs on whichever thread drops the last Python reference. Nothing drained the pool either, so parked reservations leaked across init/cleanup cycles. Leak is a straightforward bug; race is real by inspection, not reproduced.
Vendor detection missed ROCm nightlies. torch.version.hip is authoritative and is checked before the +rocm string match. Defensive; current wheels do carry +rocm.
The Windows runtime lookup checked the legacy name first. amdhip64_7.dll is now searched before the unsuffixed ROCm 5 era amdhip64.dll. Cosmetic ordering change.

Testing

Linux and APUs are the gap: Windows on a discrete RDNA4 card (gfx1200) is already covered, see below.

Nothing needs building. CI builds wheels for every push to this PR. On the Checks tab open the latest Build Wheels run, download the artifact for your platform (wheels-windows-amd64, wheels-linux-x86_64, and so on), and unzip it to get a .whl. Then, with ComfyUI closed, install it into ComfyUI's venv:

# Windows
pip install --force-reinstall --no-deps comfy_aimdo-0.4.14.dev9-cp39-abi3-win_amd64.whl

# Linux, where the manylinux tag list makes the name long
pip install --force-reinstall --no-deps comfy_aimdo-0.4.14.dev9-cp39-abi3-manylinux*.whl

The version in the file name moves with each push, so use whatever the artifact you downloaded actually contains. Downloading run artifacts requires being signed in to GitHub. To go back afterwards: pip install --force-reinstall --no-deps comfy-aimdo==<the version you had>.

If you would rather build it yourself, .github/workflows/build-wheels.yml carries the exact commands for both platforms.

Verification

CI is green on all four wheel targets, linux x86_64 and aarch64 and windows amd64 and arm64, which builds both backends on both platforms. hipDeviceAttributeIntegrated == 16 was confirmed against the ROCm headers and by querying a live gfx1200 GPU.

Run in ComfyUI on Windows on a discrete gfx1200 (RDNA4) card with a current ROCm PyTorch wheel, on workloads that offload: behaves the same as upstream. That exercises runtime and symbol resolution, the WDDM adapter match through the R0600 lookup, and the hook dedup.

It does not exercise the integrated path, which is compiled out on Windows. No Linux ROCm install or APU was available here, so that one is compiled but never run and is where testing would help most.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-aimdo/pull/89 **Author:** [@0xDELUXA](https://github.com/0xDELUXA) **Created:** 8/15/2026 **Status:** ✅ Merged **Merged:** 9/7/2026 **Merged by:** [@rattus128](https://github.com/rattus128) **Base:** `master` ← **Head:** `amd/rocm-parity` --- ### 📝 Commits (5) - [`9a5da80`](https://github.com/Comfy-Org/comfy-aimdo/commit/9a5da80b273615f2fef68781112a7693dc8a3cd8) hip: prefer the ROCm 7 runtime and correct dispatch symbol resolution - [`6bef92b`](https://github.com/Comfy-Org/comfy-aimdo/commit/6bef92b77e7a41a15feaf1401a69a964eb4850d1) shmem-detect: drop vendor specific sysmem fallback wording - [`2614512`](https://github.com/Comfy-Org/comfy-aimdo/commit/26145122f837ba39fca325a1f7ad9d1cd419a811) control: prefer torch.version.hip over the local version segment - [`8d50cea`](https://github.com/Comfy-Org/comfy-aimdo/commit/8d50ceacef5eeedfc0af2f98f981e947b2ce93ac) vrambuf: lock and tear down the rocm windows VA reservation pool - [`21e94b6`](https://github.com/Comfy-Org/comfy-aimdo/commit/21e94b643f1d2154ec98e37e6b1a06ab283b9984) control: budget integrated rocm GPUs against system RAM on linux ### 📊 Changes **8 files changed** (+88 additions, -7 deletions) <details> <summary>View changed files</summary> 📝 `comfy_aimdo/control.py` (+11 -0) 📝 `src-hip/dispatch.c` (+14 -3) 📝 `src-win/shmem-detect.c` (+1 -1) 📝 `src/control.c` (+5 -3) 📝 `src/control.h` (+2 -0) 📝 `src/gpu_abi.h` (+8 -0) 📝 `src/plat.h` (+9 -0) 📝 `src/vrambuf.c` (+38 -0) </details> ### 📄 Description Five places where the HIP backend has drifted from the CUDA one, found by diffing the two side by side. Some are plainly bugs, some are divergences, marked below. - **Integrated GPU budgeting was CUDA only.** https://github.com/Comfy-Org/comfy-aimdo/commit/159abbe landed behind a `!defined(__HIP_PLATFORM_AMD__)` guard, so APUs on ROCm get budgeted against `hipMemGetInfo` rather than `/proc/meminfo` `MemAvailable`. Enabling it needs a per-backend attribute value: `hipDeviceAttributeIntegrated` is 16, so reusing the CUDA 18 would have queried `hipDeviceAttributeKernelExecTimeout`. *Whether the `MemAvailable` heuristic suits an AMD APU needs testing.* - **The `hipGetDeviceProperties` fallback returns the wrong struct layout.** `shmem-detect.c` reads `uuid` and `luid`, which the R0000 layout does not have, so the fallback can only ever produce a bogus LUID and a failed adapter match. Removed rather than fixed. *Latent; the R0600 lookup succeeds on current runtimes.* - **The ROCm/Windows VA pool was unsynchronised and leaked.** https://github.com/Comfy-Org/comfy-aimdo/commit/2362071 added a mutex, https://github.com/Comfy-Org/comfy-aimdo/commit/c760f6a moved the pool into `AimdoContext` and dropped it, but that struct is shared across threads and `vrambuf_destroy` runs on whichever thread drops the last Python reference. Nothing drained the pool either, so parked reservations leaked across `init`/`cleanup` cycles. *Leak is a straightforward bug; race is real by inspection, not reproduced.* - **Vendor detection missed ROCm nightlies.** `torch.version.hip` is authoritative and is checked before the `+rocm` string match. *Defensive; current wheels do carry `+rocm`.* - **The Windows runtime lookup checked the legacy name first.** `amdhip64_7.dll` is now searched before the unsuffixed ROCm 5 era `amdhip64.dll`. *Cosmetic ordering change.* ## Testing Linux and APUs are the gap: Windows on a discrete RDNA4 card (gfx1200) is already covered, see below. Nothing needs building. CI builds wheels for every push to this PR. On the Checks tab open the latest Build Wheels run, download the artifact for your platform (`wheels-windows-amd64`, `wheels-linux-x86_64`, and so on), and unzip it to get a `.whl`. Then, with ComfyUI closed, install it into ComfyUI's venv: ``` # Windows pip install --force-reinstall --no-deps comfy_aimdo-0.4.14.dev9-cp39-abi3-win_amd64.whl # Linux, where the manylinux tag list makes the name long pip install --force-reinstall --no-deps comfy_aimdo-0.4.14.dev9-cp39-abi3-manylinux*.whl ``` The version in the file name moves with each push, so use whatever the artifact you downloaded actually contains. Downloading run artifacts requires being signed in to GitHub. To go back afterwards: `pip install --force-reinstall --no-deps comfy-aimdo==<the version you had>`. If you would rather build it yourself, `.github/workflows/build-wheels.yml` carries the exact commands for both platforms. ## Verification CI is green on all four wheel targets, linux x86_64 and aarch64 and windows amd64 and arm64, which builds both backends on both platforms. `hipDeviceAttributeIntegrated == 16` was confirmed against the ROCm headers and by querying a live gfx1200 GPU. Run in ComfyUI on Windows on a discrete gfx1200 (RDNA4) card with a current ROCm PyTorch wheel, on workloads that offload: behaves the same as upstream. That exercises runtime and symbol resolution, the WDDM adapter match through the R0600 lookup, and the hook dedup. It does not exercise the integrated path, which is compiled out on Windows. No Linux ROCm install or APU was available here, so that one is compiled but never run and is where testing would help most. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:24:38 +00:00