Skip to content

test(hip-tests): expect hipErrorInvalidClusterSize for a zero __clust… - #14218

Merged
sunhmy-amd merged 2 commits into
developfrom
users/mingsun/fix-cluster-zerodim-test-error-codes
Oct 28, 2026
Merged

sunhmy-amd merged 2 commits into
developfrom
users/mingsun/fix-cluster-zerodim-test-error-codes

Conversation

@sunhmy-amd

Copy link
Copy Markdown
Contributor

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

…er_dims__ axis

#13004 made a partially specified kernel cluster requirement return
hipErrorInvalidClusterSize, and made the graph path reject it at
hipGraphAddKernelNode instead of at instantiate or launch.  The two
ZeroClusterDim cases still assert the previous contract, so they fail on any
runtime that carries #13004 while Negative_OversizedCluster in the same file
already expects hipErrorInvalidClusterSize.

Require the specific code in both cases and treat node creation as a valid
rejection point.  Skipped graph stages now log "not attempted" rather than
reporting the hipSuccess they were initialised with.

Drop the claim that this matches a zero dimension passed through
hipLaunchAttributeClusterDimension: that path still returns
hipErrorInvalidConfiguration, so the two are no longer equivalent.
@sunhmy-amd
sunhmy-amd requested a review from a team as a code owner October 25, 2026 05:13
Copilot AI balanced review requested due to automatic review settings October 25, 2026 05:13
@therock-pr-bot

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/hip-tests/catch/unit/cluster/hipClusterCompilerDirective.cc; no test file found
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

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.

🟢 Approval recommended

The focused test changes match the updated runtime behavior and preserve graph control flow and cleanup, with no unresolved findings.

No open issues 🎉

What changed in this PR

Updates HIP cluster compiler-directive tests to match the runtime behavior introduced by #13004 for a zero cluster dimension.

Changes:

  • Requires hipErrorInvalidClusterSize in both direct-launch and graph tests.
  • Accepts graph rejection at node creation, instantiation, or launch, with guarded execution and clearer diagnostics.
  • Updates comments to remove the outdated equivalence with the launch-attribute route.
File Description
projects/​hip-tests/​catch/​unit/​cluster/​hipClusterCompilerDirective.cc Aligns zero-dimension assertions, graph rejection handling, and comments with the updated runtime.

Review effort: Balanced

@sunhmy-amd
sunhmy-amd merged commit d854a36 into develop Oct 28, 2026
87 of 89 checks passed
@sunhmy-amd
sunhmy-amd deleted the users/mingsun/fix-cluster-zerodim-test-error-codes branch October 28, 2026 01:23
@mangupta

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (18930e04bf58) — no new findings at or above the posting threshold.

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.
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants