Repository navigation
Implement hipSetKernelAttributeDevice API - #10542
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
c507368 to
9c7830c
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new cluster-dimension selection logic can incorrectly reject kernels with empty cluster_dims metadata, and a new attribute lock is currently taken too late in hipKernelGetAttribute, allowing data races.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds support for the new HIP runtime API hipKernelSetAttributeForDevice across HIP AMD, HIP NV, and rocprofiler-sdk, including dispatch-table/ABI wiring and unit tests. It also extends kernel-attribute handling (dynamic shared memory, carveout, cluster dims) and introduces additional synchronization around mutable per-kernel runtime attributes.
Changes:
- Add
hipKernelSetAttributeForDeviceAPI surface (headers, exports, dispatch-table/ABI, profiler IDs/args, NVIDIA backend mapping). - Implement device-specific kernel attribute storage and access, including cross-device library code-object loading and new unit tests.
- Refine cluster-dimension handling for launches/graphs and add locking for runtime-mutated kernel attributes.
File summaries
| File | Description |
|---|---|
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hip/hip.def.cpp | Registers the new HIP runtime API in rocprofiler’s runtime table. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/hip/abi.cpp | Extends dispatch table ABI enforcement/versioning for the new function pointer. |
| projects/rocprofiler-sdk/source/include/rocprofiler-sdk/hip/runtime_api_id.h | Adds a new runtime API ID enum entry. |
| projects/rocprofiler-sdk/source/include/rocprofiler-sdk/hip/api_args.h | Adds argument struct for tracing/profiling the new API. |
| projects/rocprofiler-sdk/source/include/rocprofiler-sdk/cxx/enum_string.hpp | Adds enum-to-string label + updates static_assert versioning. |
| projects/hipother/hipnv/include/hip/nvidia_detail/nvidia_hip_runtime_api.h | Adds CUDA-version-gated NV implementation wrapper. |
| projects/hip/include/hip/hip_runtime_api.h | Declares the new public HIP API with doxygen docs. |
| projects/hip-tests/catch/unit/library/lib_attribute.cc | Adds coverage for function-vs-kernel attribute override behavior. |
| projects/hip-tests/catch/unit/library/hipKernelSetAttributeForDevice.cc | Adds new unit tests for the device-specific attribute API. |
| projects/hip-tests/catch/unit/library/CMakeLists.txt | Adds the new unit test source to the build. |
| projects/hip-tests/catch/config/configs/unit/library.yaml | Registers the new unit tests in the test config. |
| projects/clr/rocclr/platform/ndrange.hpp | Tracks whether cluster dims were explicitly specified vs implicit default. |
| projects/clr/rocclr/device/rocm/rocvirtual.cpp | Synchronizes attribute reads and preserves -1/0 semantics for carveout. |
| projects/clr/rocclr/device/rocm/rockernel.cpp | Initializes new “kernel-level” dynamic shared memory fields. |
| projects/clr/rocclr/device/pal/palkernel.cpp | Initializes new “kernel-level” dynamic shared memory fields (PAL path). |
| projects/clr/rocclr/device/devkernel.hpp | Adds runtime attribute fields + a mutex for attribute serialization. |
| projects/clr/rocclr/device/devkernel.cpp | Initializes new runtime attribute fields and defaults. |
| projects/clr/hipamd/src/hip_table_interface.cpp | Adds public forwarding stub through the dispatch table. |
| projects/clr/hipamd/src/hip_platform.cpp | Plumbs explicit-cluster-dims tracking through launch path. |
| projects/clr/hipamd/src/hip_module.cpp | Adds graph cluster-dim resolver and updates cluster selection/validation. |
| projects/clr/hipamd/src/hip_library.hpp | Adds per-device code-object cache and a “resolve kernel for device” helper. |
| projects/clr/hipamd/src/hip_library.cpp | Implements hipKernelSetAttributeForDevice and cross-device kernel resolution. |
| projects/clr/hipamd/src/hip_hcc.map.in | Exports the new symbol in the version script. |
| projects/clr/hipamd/src/hip_graph.cpp | Uses sentinel {0,0,0} + resolver for graph cluster-dim defaults. |
| projects/clr/hipamd/src/hip_graph_internal.hpp | Resolves/validates graph cluster dims when setting node attributes. |
| projects/clr/hipamd/src/hip_graph_helper.hpp | Declares the new graph cluster-dim resolver helper. |
| projects/clr/hipamd/src/hip_global.hpp | Extends dyn-function resolution API to be device-ID aware. |
| projects/clr/hipamd/src/hip_global.cpp | Implements device-ID-aware dyn-function caching. |
| projects/clr/hipamd/src/hip_code_object.hpp | Adds device-ID ctor + optional global-var init in code-object loading. |
| projects/clr/hipamd/src/hip_code_object.cpp | Implements per-device code-object loading with optional global-var init. |
| projects/clr/hipamd/src/hip_api_trace.cpp | Wires the new dispatch entry + bumps runtime table step version. |
| projects/clr/hipamd/src/amdhip.def.in | Exports the new API from the Windows def list. |
| projects/clr/hipamd/include/hip/amd_detail/hip_prof_str.h | Adds profiler API ID/name + trace argument wiring. |
| projects/clr/hipamd/include/hip/amd_detail/hip_api_trace.hpp | Bumps runtime table step version and adds new dispatch function type/slot. |
Review details
- Files reviewed: 34/34 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate issues affect safety, concurrency, device behavior, and API correctness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
projects/clr/hipamd/src/hip_library.cpp:619
- These runtime cluster dimensions are only consumed by the launch/graph paths.
hipOccupancyMaxActiveClustersstill resolves dimensions throughclusterDimensions, which reads onlywrkGrpInfo.clusterSize_and ignoresruntimeClusterSize_; after setting all three required dimensions here, an occupancy query without a launch attribute therefore still reportshipErrorInvalidClusterSizeinstead of using the device-specific setting. Update the occupancy path to use the effective per-device cluster dimensions.
work_group_info->runtimeClusterSize_[index] = value;
projects/clr/hipamd/src/hip_library.cpp:113
- The secondary
DynCOcreated here is only used to service this setter/getter; its kernel handle is not registered as the library's per-device handle, and the normal launch path still receives the primaryhipKernel_t/hipFunction_t. Since the primary program has nodevice_kernelentry for the secondary device, switching to that device after setting this attribute cannot launch the configured library kernel (andhipLibraryGetKernelstill returns the primary handle). Persist the per-device handle and resolve it from kernel retrieval/launch, or otherwise make the target attributes part of the handle used by launches.
IHIP_RETURN_ONFAIL(target->loadCodeObject(fname, image, init_global_vars));
projects/clr/hipamd/src/hip_module.cpp:549
hasClusterAttr_is also set for an empty__cluster_dims__()tuple, whereclusterSize_is{0,0,0}. In that caserequired_clusterremains all zero, so theelse ifbelow returnshipErrorInvalidClusterSizefor every ordinary launch that does not explicitly pass cluster dimensions; the graph resolver already treats this tuple as the no-dimensions/default case. Gate this flag on at least one nonzero compile-time component while preservinghas_runtime_cluster, so ordinary launches remain valid.
const bool has_required_cluster = work_group_info->hasClusterAttr_ || has_runtime_cluster;
projects/hip/include/hip/hip_runtime_api.h:2905
- This contract is not implemented for the cluster attributes listed above:
hipFuncSetAttributeonly records the max-dynamic-shared-memory and carveout overrides, while cluster width/height/depth, non-portable cluster size, and scheduling policy currently fall through withhipSuccesswithout storing a function-level override. Consequently, ahipFuncSetAttributecall does not overridehipKernelSetAttributeForDevicefor those attributes despite this documentation promise; either implement the corresponding precedence state or narrow the supported/override documentation.
* Attributes set with #hipFuncSetAttribute override values set by this function
* for the corresponding device, regardless of call order.
- Files reviewed: 36/36 changed files
- Comments generated: 2
- Review effort level: Lite
de6b1e5 to
e99fb88
Compare
1f03d5a to
a1be69e
Compare
a1be69e to
ff2020d
Compare
ff2020d to
bc283c3
Compare
mangupta
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @mangupta)
[IMPORTANT] New public API hipKernelSetAttributeForDevice has no contract test, no uncovered_apis.txt rationale, and no TEST_PLAN.md update
The PR adds a new public HIP entry point (projects/hip/include/hip/hip_runtime_api.h, amdhip.def.in, hip_hcc.map.in, dispatch table step 35) but touches no file under projects/hip-tests/catch/contract/, adds no @asserts: API - invariant annotation, adds no rationale to catch/contract/uncovered_apis.txt, and does not update the generated catch/TEST_PLAN.md.
Suggested fix: Add a projects/hip-tests/catch/contract/ test asserting the documented invariants of hipKernelSetAttributeForDevice (e.g. @asserts: hipKernelSetAttributeForDevice - setting an attribute for a non-current device does not change the current device and is visible via hipKernelGetAttribute for that device), or record a concrete rationale in catch/contract/uncovered_apis.txt, and regenerate catch/TEST_PLAN.md.
30aef7f to
2e02ba9
Compare
8b77830 to
3d916b3
Compare
Keep hipLibraryLoadData bytes after the first build so later per-device attribute loads do not reread a caller buffer that may already be gone. Co-authored-by: Cursor <cursoragent@cursor.com>
3d916b3 to
ce8b8c6
Compare
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
## Motivation gfx950 hip-tests shard 2 fails `Unit_hipKernelSetAttribute_Positive_FunctionOverride` on both attempts of TheRock #8774. The test reads `kernel_value` 0 and expects 1024. That test and the `hipSetKernelAttributeDevice` API 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 `7a759225bf580bd454e4a83279da4344b5efd0c8` in 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, including `Unit_hipKernelSetAttribute_Positive_FunctionOverride` in `projects/hip-tests/catch/unit/library/lib_attribute.cc` and `hipKernelSetAttributeForDevice.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_FunctionOverride` because 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 - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. Made with [Cursor](https://cursor.com)
TheRock Submodule Bump ActivityNewest first
|
#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.
#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.



Motivation
Implement hipSetKernelAttributeDevice API. The API sets a kernel attribute for an explicit device without making that device current.
Technical Details
New api hipKernelSetAttributeForDevice(kernel, attr, value, device) allows different attributes to be set including max dynamic shared memory, preferred shared-memory carveout, required cluster width/height/depth, non-portable cluster size, and cluster scheduling policy. Values are stored per (kernel, device). hipFuncSetAttribute still overrides them for launch, regardless of call order.
If the requested device is not the one used for the first library load, we lazily loads that device’s code object from the same library source. For hipLibraryLoadData, the first build copies the in-memory image so a later device load still works after the caller releases the original buffer. File-backed libraries reopen the owned filename.
Issue Tracking
JIRA ID: AIRUNTIME-2607
Test Plan
LibraryTests: Unit_hipKernelSetAttributeForDevice* and Unit_hipKernelGetAttribute*
Test Result
Tests are passing.
Submission Checklist