Repository navigation
Remove uses of hipconfig: derive HIP version from find_package(hip) - #5708
Merged
Merged
Conversation
nileshnegi
approved these changes
May 9, 2026
nileshnegi
left a comment
nileshnegi
left a comment
Contributor
There was a problem hiding this comment.
Approved for changes to RCCL and RCCL-Tests
david-salinas
force-pushed
the
users/dsalinas/deprecate-hipcc
branch
from
May 17, 2026 19:53
886e32c to
5d26b14
Compare
1 task
david-salinas
marked this pull request as ready for review
May 26, 2026 17:27
lamb-j
reviewed
Jun 2, 2026
lamb-j
reviewed
Jun 2, 2026
lamb-j
changed the title
accauble
force-pushed
the
users/dsalinas/deprecate-hipcc
branch
from
June 9, 2026 15:21
1a9a173 to
dabc306
Compare
agunashe
reviewed
Jun 9, 2026
agunashe
reviewed
Jun 9, 2026
sauverma93
reviewed
Jun 11, 2026
When CMAKE_HIP_ARCHITECTURES is set to OFF without an alternative, test kernels compile only for clang's default arch. Override with GPU_TARGETS (injected by TheRock's toolchain) so --offload-arch flags match the intended target architectures. Addresses amd-argus review feedback on PR #5708. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 1,879 lines across 32 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.
Bottom line: Worth settling before approval: a space-separated GPU_TARGETS (the build.sh default) becomes one invalid CMAKE_HIP_ARCHITECTURES entry at roctracer/test/CMakeLists.txt:31 and the four rocprofiler copies.
Comment @argus review to re-run, or @argus re-review after pushing.
GPU_TARGETS may arrive space-separated (e.g. from build.sh) but CMAKE_HIP_ARCHITECTURES requires a semicolon-delimited CMake list. Use string(REPLACE) to convert spaces so clang receives correct --offload-arch flags instead of treating extra targets as input files. Addresses amd-argus review feedback on PR #5708. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
Contributor
There was a problem hiding this comment.
ARGUS
This PR changes 1,879 lines across 32 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.
Bottom line: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
Contributor
pr-rocm-systems-5708-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-5708-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
Contributor
pr-rocm-systems-5708-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-5708-oci-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-5708-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-5708-ruby64-rccl-pr-smokeBuilds
Failures
200 tests blocked and 15 tests skipped. |
Contributor
pr-rocm-systems-5708-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-5708-oci-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-5708-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-5708-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-5708-ruby64-rccl-pr-smokeBuilds
Failures
200 tests blocked and 15 tests skipped. |
Contributor
pr-rocm-systems-5708-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-5708-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Collaborator
|
I created a therock test-build to verify that these changes does not break it and it passed: tensilelite, rocfft and rocsparse errors is known issue in quick mode: |
lamikr
approved these changes
Oct 29, 2026
lamikr
left a comment
lamikr
left a comment
Collaborator
There was a problem hiding this comment.
Review suggestions have been applied tests are passing (except unrelated known 3 failures) and also therock test build passed with these changes included.
Contributor
pr-rocm-systems-5708-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-5708-oci-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-5708-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-5708-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-5708-ruby64-rccl-pr-smokeBuilds
Failures
200 tests blocked and 15 tests skipped. |
Contributor
pr-rocm-systems-5708-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-5708-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-5708-ruby64-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 183 tests skipped. pr-rocm-systems-5708-ruby64-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
TheRock Submodule Bump ActivityNewest first
|
2 tasks done
This was referenced Nov 2, 2026
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Remove functional calls to hipconfig and legacy hipcc wrapper scripts from the rocm-systems stack. hipconfig is a legacy companion to the hipcc wrapper and is being deprecated in favor of CMake-native HIP support (find_package(hip CONFIG)) and direct file-based alternatives. See the
https://rocm.docs.amd.com/en/latest/reference/rocmcc.html for background.
Technical Details
HIP
CLR
RCCL
Also adds missing --offload-host-only and -no-hip-rt flags to rccl-source-wrappers.
HIP Tests
ROCprofiler
ROCtracer
Issue Tracking
JIRA ID: ROCM-8370
Test Plan
Test Result
Submission Checklist