Repository navigation
revert(hip): revert hipSetKernelAttributeDevice API - #12954
Merged
JeniferC99 merged 1 commit intoOct 6, 2026
Merged
Conversation
This reverts commit 7a75922. Reason: gfx950 hip-tests shard 2 fails Unit_hipKernelSetAttribute_Positive_FunctionOverride (kernel_value 0, expected 1024) on both attempts. Impact: TheRock rocm-systems bump CI is blocked on that gfx950 hip-tests shard. CI Run: https://github.com/ROCm/TheRock/actions/runs/37406623340 Tracking: #12950
JeniferC99
requested review from
amd-aakash
and
a balanced review from Copilot
October 6, 2026 20:01
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes consistently reverse the referenced commit, and no remaining references to the removed API or implementation fields were found.
Review effort: Balanced
Findings: None
What changed in this PR
Reverts the recently added hipKernelSetAttributeForDevice API and associated runtime behavior to unblock gfx950 testing.
Changes:
- Removes the API across HIP, NVIDIA compatibility, tracing, and rocprofiler-sdk.
- Restores pre-implementation kernel, cluster-launch, and code-object behavior.
- Removes associated unit/contract tests and updates coverage metadata.
| File | Description |
|---|---|
projects/clr/hipamd/include/hip/amd_detail/hip_api_trace.hpp |
Rolls back dispatch-table version 36. |
projects/clr/hipamd/include/hip/amd_detail/hip_prof_str.h |
Removes tracing metadata for the API. |
projects/clr/hipamd/src/amdhip.def.in |
Removes the exported symbol. |
projects/clr/hipamd/src/hip_api_trace.cpp |
Removes dispatch and ABI registration. |
projects/clr/hipamd/src/hip_code_object.cpp |
Restores current-device code-object loading. |
projects/clr/hipamd/src/hip_code_object.hpp |
Restores the prior DynCO interface. |
projects/clr/hipamd/src/hip_fatbin.cpp |
Removes image preservation for secondary-device loading. |
projects/clr/hipamd/src/hip_fatbin.hpp |
Restores the prior extraction signature. |
projects/clr/hipamd/src/hip_global.cpp |
Restores current-device dynamic-function resolution. |
projects/clr/hipamd/src/hip_global.hpp |
Restores the prior function interface. |
projects/clr/hipamd/src/hip_graph.cpp |
Restores prior graph cluster handling. |
projects/clr/hipamd/src/hip_graph_helper.hpp |
Removes the reverted cluster resolver declaration. |
projects/clr/hipamd/src/hip_graph_internal.hpp |
Removes runtime cluster-requirement validation. |
projects/clr/hipamd/src/hip_hcc.map.in |
Removes the versioned API export. |
projects/clr/hipamd/src/hip_library.cpp |
Removes per-device library kernel resolution. |
projects/clr/hipamd/src/hip_library.hpp |
Removes secondary-device library state. |
projects/clr/hipamd/src/hip_module.cpp |
Restores prior attributes and cluster-launch behavior. |
projects/clr/hipamd/src/hip_platform.cpp |
Restores the prior launch interface. |
projects/clr/hipamd/src/hip_table_interface.cpp |
Removes the public table wrapper. |
projects/clr/rocclr/device/devkernel.cpp |
Removes added attribute-state initialization. |
projects/clr/rocclr/device/devkernel.hpp |
Removes added per-kernel attribute fields. |
projects/clr/rocclr/device/pal/palkernel.cpp |
Restores prior dynamic shared-memory initialization. |
projects/clr/rocclr/device/rocm/rockernel.cpp |
Restores prior dynamic shared-memory initialization. |
projects/clr/rocclr/device/rocm/rocvirtual.cpp |
Restores prior LDS carveout selection. |
projects/clr/rocclr/platform/ndrange.hpp |
Removes explicit cluster-dimension tracking. |
projects/hip-tests/CONTRACT_COVERAGE.md |
Updates contract coverage totals. |
projects/hip-tests/catch/TEST_PLAN.md |
Removes reverted contract cases. |
projects/hip-tests/catch/config/configs/contract.yaml |
Removes contract test registrations. |
projects/hip-tests/catch/config/configs/unit/library.yaml |
Removes reverted unit-test registrations. |
projects/hip-tests/catch/contract/kernel_object_attributes/test_hip_kernel_object_attributes_contract.cc |
Removes API contract tests. |
projects/hip-tests/catch/unit/library/CMakeLists.txt |
Removes the API test source. |
projects/hip-tests/catch/unit/library/hipKernelSetAttributeForDevice.cc |
Deletes API-specific unit tests. |
projects/hip-tests/catch/unit/library/lib_attribute.cc |
Removes the failing override test. |
projects/hip/include/hip/hip_runtime_api.h |
Removes the public API declaration. |
projects/hipother/hipnv/include/hip/nvidia_detail/nvidia_hip_runtime_api.h |
Removes NVIDIA mappings and implementation. |
projects/rocprofiler-sdk/source/include/rocprofiler-sdk/cxx/enum_string.hpp |
Removes API enum labeling. |
projects/rocprofiler-sdk/source/include/rocprofiler-sdk/hip/api_args.h |
Removes captured API arguments. |
projects/rocprofiler-sdk/source/include/rocprofiler-sdk/hip/runtime_api_id.h |
Removes the runtime API identifier. |
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hip/abi.cpp |
Removes step-36 ABI enforcement. |
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hip/hip.def.cpp |
Removes profiler API metadata. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
amd-aakash
approved these changes
Oct 6, 2026
amd-aakash
left a comment
amd-aakash
left a comment
Collaborator
There was a problem hiding this comment.
LGTM this is to unblock the bump-pr merge
JeniferC99
deleted the
users/jechrist/revert-hip-set-kernel-attribute-device
branch
October 6, 2026 20:44
1 task done
1 task
sunhmy-amd
added a commit
that referenced
this pull request
Oct 28, 2026
#14218) ## Motivation `Unit_hipClusterLaunch_CompilerDirective_Negative_ZeroClusterDim` and its `_Graph` variant fail on any runtime carrying #13004. They passed on ROCm 10.2.0a build 26405 and fail on 26422; the only exported-symbol difference between those two `libamdhip64` builds is `hipKernelSetAttributeForDevice@@hip_7.16`, which #13004 adds. The launch is still correctly refused — only the error code and, for graphs, the rejection point changed. `Negative_OversizedCluster` in the same file already expects `hipErrorInvalidClusterSize` and passes, so the two ZeroClusterDim cases are the stragglers. ## Technical Details The kernel is `CLUSTER_DIMS(0, 1, 1)`, i.e. a zero in one axis only. Direct launch — `UpdateNumClustersFromKernel`, `projects/clr/hipamd/src/hip_module.cpp:552-577`: required_cluster[i] = hasClusterAttr_ && clusterSize_[i] != 0 ? clusterSize_[i] : runtimeClusterSize_[i]; // -> {0, 1, 1} `<<<nbig, ntib>>>` supplies no dimensions, so `clusterDimsSpecified_` is false and `has_required_cluster` is true. That branch copies the requirement into `selected_cluster` and returns `hipErrorInvalidClusterSize` on the zero component. The test asserted `hipErrorInvalidConfiguration`. Graph — `ihipGraphAddKernelNode` calls `ihipResolveGraphClusterDimensions` (`hip_module.cpp:481-483`) before building launch params (`hip_graph.cpp:128-133`), and the resolver rejects a partially specified requirement outright. So `hipGraphAddKernelNode` now returns `hipErrorInvalidClusterSize`. On 26405 the node-add succeeds and `hipGraphInstantiate` returns `hipErrorInvalidConfiguration` instead. The test wrapped the node-add in `HIP_CHECK` and only tolerated a rejection at instantiate or launch, so it fails the moment the rejection moves earlier. This change requires `hipErrorInvalidClusterSize` in both cases and adds node creation to the set of valid rejection points, matching `Negative_OversizedCluster` three tests above. Requiring the specific code rather than also admitting `hipErrorInvalidConfiguration` keeps the test able to catch a regression back to the generic code; it does mean these cases now require a runtime carrying #13004, which is the pairing CI builds. The doc comment also drops its claim that this matches a zero dimension passed through `hipLaunchAttributeClusterDimension`. #13004 moved only the compiler-directive route, so the two no longer agree — measured directly on both stacks: route 26405 26422 __cluster_dims__(0,1,1) 9 1054 attr clusterDim {0,1,1} 9 9 Whether the attribute route should follow is a CLR question, so this change leaves it alone and only stops the comment from asserting an equivalence that no longer holds. ## Issue Tracking JIRA ID: none — no ticket exists for either failure. Behaviour changed by: #13004 (re-land of #10542, reverted by #12954) ## Test Plan gfx1250, two ROCm 10.2.0a tarball stacks in a container. First the shipped test binaries against their own libraries, plus single-library overrides to attribute the change. Then `ClusterTest` built from this branch and run against each stack, whole binary, to confirm no other case moves: ROCR_VISIBLE_DEVICES=0 ./ClusterTest \ "Unit_hipClusterLaunch_CompilerDirective_Negative_ZeroClusterDim[_Graph]" ## Test Result Component isolation, one library overridden per arm (`ldd` captured per run to confirm the override took): | libamdhip64 | libamd_comgr | libhsa-runtime64 | result | |---|---|---|---| | 26405 | 26405 | 26405 | pass | | 26422 | 26422 | 26422 | fail `1054 == 9` | | 26405 | 26422 | 26422 | pass | | 26422 | 26405 | 26405 | fail `1054 == 9` | | 26422 | 26405 | 26422 | fail | | 26405 | 26422 | 26405 | pass | The verdict follows `libamdhip64` in every arm; ROCr, comgr and the test binary are not the variable. The 26422 test binary passes on 26405 libraries and the 26405 test binary fails on 26422 libraries. Identical at 1, 2 and 4 visible GPUs. Observed in a full `hip_directed` run on both stacks: 26405 1 failure / 5054, 26422 3 failures / 5080, the two extra being these cases. The third failure, `Unit_hipDeviceFlushGPUDirectRDMAWrites_CapabilityConsistency`, fails on both stacks and is unrelated. `ClusterTest` built from this branch, whole binary: 26422 8 cases | 7 passed | 1 skipped 404 assertions | 404 passed 26405 8 cases | 5 passed | 2 failed | 1 skipped 404 assertions | 402 passed | 2 failed Both target cases pass on 26422 and no other case moves. On 26405 they fail `9 == 1054`, which is the intended signal: that runtime predates #13004 and returns the generic code. CI pairs hip-tests with a matching CLR, so this only shows up when a newer test binary is deliberately run against an older library set. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
ywang103-amd
pushed a commit
that referenced
this pull request
Oct 28, 2026
#14218) ## Motivation `Unit_hipClusterLaunch_CompilerDirective_Negative_ZeroClusterDim` and its `_Graph` variant fail on any runtime carrying #13004. They passed on ROCm 10.2.0a build 26405 and fail on 26422; the only exported-symbol difference between those two `libamdhip64` builds is `hipKernelSetAttributeForDevice@@hip_7.16`, which #13004 adds. The launch is still correctly refused — only the error code and, for graphs, the rejection point changed. `Negative_OversizedCluster` in the same file already expects `hipErrorInvalidClusterSize` and passes, so the two ZeroClusterDim cases are the stragglers. ## Technical Details The kernel is `CLUSTER_DIMS(0, 1, 1)`, i.e. a zero in one axis only. Direct launch — `UpdateNumClustersFromKernel`, `projects/clr/hipamd/src/hip_module.cpp:552-577`: required_cluster[i] = hasClusterAttr_ && clusterSize_[i] != 0 ? clusterSize_[i] : runtimeClusterSize_[i]; // -> {0, 1, 1} `<<<nbig, ntib>>>` supplies no dimensions, so `clusterDimsSpecified_` is false and `has_required_cluster` is true. That branch copies the requirement into `selected_cluster` and returns `hipErrorInvalidClusterSize` on the zero component. The test asserted `hipErrorInvalidConfiguration`. Graph — `ihipGraphAddKernelNode` calls `ihipResolveGraphClusterDimensions` (`hip_module.cpp:481-483`) before building launch params (`hip_graph.cpp:128-133`), and the resolver rejects a partially specified requirement outright. So `hipGraphAddKernelNode` now returns `hipErrorInvalidClusterSize`. On 26405 the node-add succeeds and `hipGraphInstantiate` returns `hipErrorInvalidConfiguration` instead. The test wrapped the node-add in `HIP_CHECK` and only tolerated a rejection at instantiate or launch, so it fails the moment the rejection moves earlier. This change requires `hipErrorInvalidClusterSize` in both cases and adds node creation to the set of valid rejection points, matching `Negative_OversizedCluster` three tests above. Requiring the specific code rather than also admitting `hipErrorInvalidConfiguration` keeps the test able to catch a regression back to the generic code; it does mean these cases now require a runtime carrying #13004, which is the pairing CI builds. The doc comment also drops its claim that this matches a zero dimension passed through `hipLaunchAttributeClusterDimension`. #13004 moved only the compiler-directive route, so the two no longer agree — measured directly on both stacks: route 26405 26422 __cluster_dims__(0,1,1) 9 1054 attr clusterDim {0,1,1} 9 9 Whether the attribute route should follow is a CLR question, so this change leaves it alone and only stops the comment from asserting an equivalence that no longer holds. ## Issue Tracking JIRA ID: none — no ticket exists for either failure. Behaviour changed by: #13004 (re-land of #10542, reverted by #12954) ## Test Plan gfx1250, two ROCm 10.2.0a tarball stacks in a container. First the shipped test binaries against their own libraries, plus single-library overrides to attribute the change. Then `ClusterTest` built from this branch and run against each stack, whole binary, to confirm no other case moves: ROCR_VISIBLE_DEVICES=0 ./ClusterTest \ "Unit_hipClusterLaunch_CompilerDirective_Negative_ZeroClusterDim[_Graph]" ## Test Result Component isolation, one library overridden per arm (`ldd` captured per run to confirm the override took): | libamdhip64 | libamd_comgr | libhsa-runtime64 | result | |---|---|---|---| | 26405 | 26405 | 26405 | pass | | 26422 | 26422 | 26422 | fail `1054 == 9` | | 26405 | 26422 | 26422 | pass | | 26422 | 26405 | 26405 | fail `1054 == 9` | | 26422 | 26405 | 26422 | fail | | 26405 | 26422 | 26405 | pass | The verdict follows `libamdhip64` in every arm; ROCr, comgr and the test binary are not the variable. The 26422 test binary passes on 26405 libraries and the 26405 test binary fails on 26422 libraries. Identical at 1, 2 and 4 visible GPUs. Observed in a full `hip_directed` run on both stacks: 26405 1 failure / 5054, 26422 3 failures / 5080, the two extra being these cases. The third failure, `Unit_hipDeviceFlushGPUDirectRDMAWrites_CapabilityConsistency`, fails on both stacks and is unrelated. `ClusterTest` built from this branch, whole binary: 26422 8 cases | 7 passed | 1 skipped 404 assertions | 404 passed 26405 8 cases | 5 passed | 2 failed | 1 skipped 404 assertions | 402 passed | 2 failed Both target cases pass on 26422 and no other case moves. On 26405 they fail `9 == 1054`, which is the intended signal: that runtime predates #13004 and returns the generic code. CI pairs hip-tests with a matching CLR, so this only shows up when a newer test binary is deliberately run against an older library set. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.
1 task done
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
gfx950 hip-tests shard 2 fails
Unit_hipKernelSetAttribute_Positive_FunctionOverrideon both attempts of TheRock #8774. The test readskernel_value0 and expects 1024. That test and thehipSetKernelAttributeDeviceAPI landed together in #10542 (7a759225bf). The author has been told. This revert restores the pre-#10542 behavior so the rocm-systems bump is not blocked on that shard.Technical Details
Reverts
7a759225bf580bd454e4a83279da4344b5efd0c8in full (40 files). That removes the device kernel-attribute API from hipamd, rocclr, the HIP runtime headers, the rocprofiler-sdk HIP ABI entries, and the hip-tests coverage added with it, includingUnit_hipKernelSetAttribute_Positive_FunctionOverrideinprojects/hip-tests/catch/unit/library/lib_attribute.ccandhipKernelSetAttributeForDevice.cc.The revert applies cleanly on current
develop. A search of the tree after the revert finds no remaining callers of the removed API.JIRA ID
ISSUE ID : #12950
Test Plan
CI on this PR. The gfx950 hip-tests shard 2 job should get past
Unit_hipKernelSetAttribute_Positive_FunctionOverridebecause the test is removed with the API.Test Result
Not run locally. The change is a mechanical revert of
7a759225bf(215 insertions, 1287 deletions).Submission Checklist
Made with Cursor