Skip to content

[Build] Find ROCm through rocm-sdk before falling back to /opt/rocm* - #1142

Merged
coderfeli merged 5 commits into
ROCm:mainfrom
xinyazhang:xinyazhang/theRock-support
Sep 29, 2026
Merged

coderfeli merged 5 commits into
ROCm:mainfrom
xinyazhang:xinyazhang/theRock-support

Conversation

@xinyazhang

@xinyazhang xinyazhang commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Since ROCm 7.14, ROCm can be installed through pip. The HIP package used to build
FlyDSL must come from the selected ROCm installation. Passing a ROCm prefix in
find_package(hip ... PATHS ...) did not guarantee this: CMake searches its
normal package locations before PATHS, so a system HIP could win over the SDK
in the active virtual environment.

Change

The ROCm runtime now tries HIP in this order:

  1. ROCM_PATH, if it resolves a HIP package;
  2. the root reported by rocm-sdk path --root, if that command succeeds and
    resolves a HIP package;
  3. CMake's normal package locations and the existing /opt/rocm* fallback.

The first two searches use CMake's NO_DEFAULT_PATH option so defaults cannot
precede the selected prefix. An empty or stale ROCM_PATH falls through to the
SDK. While loading HIP from the SDK, ROCM_PATH temporarily points to that SDK
so HIP's own dependencies are found there too; its original value is restored
afterwards. If rocm-sdk is missing or fails, the original fallback still works.

As with other CMake packages, an explicit or previously cached hip_DIR remains
authoritative. Clear hip_DIR or use a fresh build directory when switching
ROCm installations.

Validation

  • The original PR built flydsl-0.3.1.dev1129-cp313-cp313-linux_x86_64.whl
    in a Debian 13 container with a ROCm 7.14 virtual environment.
  • Configure checks with the installed pip ROCm SDK and a fake system HIP
    confirm that unset, empty, and stale ROCM_PATH values select the SDK.
    A valid ROCM_PATH wins, and a failed rocm-sdk command falls back to
    normal CMake package discovery.
  • The existing CMake backend tests pass (4 tests).
  • After updating from main, a local Sphinx build with warnings treated as
    errors passed. This includes the docs change that removes the remote mapping
    request which had hit a GitHub API rate limit on the older branch.
  • CI on the earlier search-order fix built a PR wheel; MI325, MI35x, and Navi
    GPU tests and benchmarks passed. Navi passed on retry after its runner failed
    to download the wheel artifact on the first attempt. Checks for the current
    head are shown below.

Submission Checklist

`find_package(hip)` was pointed at a `/opt/rocm*` glob, so a pip-installed ROCm
SDK inside a virtualenv was invisible and the build either picked up an
unrelated system ROCm or failed outright.

Order is now `ROCM_PATH`, then `rocm-sdk path --root` — the SDK's own query,
whose root carries `lib/cmake/hip/hip-config.cmake` — then the original glob.
`ERROR_QUIET` plus the empty-output check covers "rocm-sdk absent" and
"rocm-sdk failed" the same way, since both fall through.

Verified in a scratch project against the real file: resolves via `rocm-sdk`,
resolves via `ROCM_PATH` and takes precedence, and with neither available falls
back to the glob and then fails at `find_package` — which is right on a host
with no system ROCm.

This was the only `/opt/rocm` hardcode in the repo's CMake.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@xinyazhang
xinyazhang requested a lite review from Copilot September 15, 2026 18:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@xinyazhang
xinyazhang marked this pull request as ready for review September 15, 2026 18:45

@coderfeli coderfeli left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the updated HIP discovery change. The real pip ROCm HIP config reads ROCM_PATH when resolving dependencies, so an empty or stale value previously caused configure to fail at AMDDeviceLibs even after hip_DIR selected the SDK. Commit f39f41b scopes ROCM_PATH to the SDK during that lookup and restores it afterward. I also removed the mock-only discovery test.

I configured against the installed pip ROCm SDK with ROCM_PATH unset, empty, and stale; all three now select the SDK. A valid explicit prefix and the normal fallback after a failed rocm-sdk command also resolved as expected. The four existing CMake backend tests pass. I found no remaining blocking issue in this diff; CI for the updated head is still pending.

@coderfeli
coderfeli merged commit 1941889 into ROCm:main Sep 29, 2026
11 checks passed
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.

3 participants