Skip to content

test(hip-kernel-provider): compare workspaces instead of hardcoding a device-dependent size - #12478

Merged
BrianHarrisonAMD merged 2 commits into
developfrom
users/bharriso/layernorm-workspace-test-shape
Sep 24, 2026
Merged

BrianHarrisonAMD merged 2 commits into
developfrom
users/bharriso/layernorm-workspace-test-shape

Conversation

@BrianHarrisonAMD

@BrianHarrisonAMD BrianHarrisonAMD commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #12479.

Two layernorm backward workspace tests pass or fail depending on the compute-unit count of the machine running them. Any PR can be blocked at random, and they are equally broken on develop — the green runs simply landed on machines outside the failing band.

  • TestLayernormBwdPlan.GetWorkspaceSizeWithoutOptionalTensorsReturnsNonzero
  • TestLayernormPlanBuilder.GetMaxWorkspaceSizeBpropWithoutOptionalTensorsReturnsNonzero

Example failure: run 35807379268, job 107050043339 — Which is: 2408464 vs the expected 16u.

Root cause

The backward workspace is the sum of two terms (LayernormBwdPlan.cpp:148-169, helpers at :344-372):

reqd       = 4 * localSize * multiProcessorCount      // localSize = MAX_LOCAL_SIZE = 1024
isParallel = innerSize <= reqd && innerSize * outerSize > reqd
workspace  = 4 * (parallel ? 2*innerSize*parallelSize : 0)       // parallel reduction buffer
           + 4 * (no mean/inv_variance ? 2*outerSize*stride : 0) // fallback mean/rstd buffer

Both tests want the second term, and assert it as an absolute 16u. That only holds when the first term is zero, which for their shape (outerSize=2, innerSize=150528) means a CU count outside the band [37, 73]. Inside that band the parallel branch is taken and adds 2408448 bytes, which is the failure above. Whether a given run lands inside the band is a property of the machine, not of the change under test.

Change

Assert the difference between the same shape with and without mean/inv_variance. Both graphs have identical dims and strides, so they take the same branch and carry the same parallel reduction buffer — it cancels, leaving the fallback mean/rstd term alone:

EXPECT_EQ(planWithoutStats.getWorkspaceSize(handle) - planWithStats.getWorkspaceSize(handle), 16u);

No CU-dependent expected value, no assumption about which branch the machine takes, and the original shape is kept so both branches still get exercised by whichever machines run the suite. Test-only; no product code changes.

Verification

Full hip_kernel_provider_tests unit suite on the development machine: 531 tests, 524 passed, 7 skipped, 0 failed.

The cancellation was checked with the parallel branch live, using a throwaway probe (not committed) with dims {8,3,224,224} — a shape that satisfies isParallel on that machine's CU count:

withStats=4816896 withoutStats=4816960 delta=64

withStats is the parallel reduction buffer alone (nonzero, so the branch is active) and the delta is exactly 4 * 2 * outerSize * stride = 4 * 2 * 8 * 1 = 64 — the fallback term, independent of the parallel term.

The original CU dependence was reproduced by driving the public statics with fabricated hipDeviceProp_t values:

CU= 16 | parallel=0 ws=16
CU= 27 | parallel=0 ws=16
CU= 48 | parallel=1 ws=2408464
CU= 60 | parallel=1 ws=2408464
CU=104 | parallel=0 ws=16
CU=304 | parallel=0 ws=16

ws=2408464 inside the band matches the linked CI failure byte for byte.

@therock-pr-bot

therock-pr-bot Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass PR does not contain code files — Unit Test auto-passed
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

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

🙋 Wish to Override Policy?

@therock-pr-bot

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

@BrianHarrisonAMD
BrianHarrisonAMD force-pushed the users/bharriso/layernorm-workspace-test-shape branch from 8e82819 to e48f676 Compare September 23, 2026 14:29
@BrianHarrisonAMD BrianHarrisonAMD changed the title test(hip-kernel-provider): pin layernorm bwd workspace tests to the serial path test(hip-kernel-provider): compare workspaces instead of hardcoding a device-dependent size Sep 23, 2026
… device-dependent size

`GetWorkspaceSizeWithoutOptionalTensorsReturnsNonzero` and
`GetMaxWorkspaceSizeBpropWithoutOptionalTensorsReturnsNonzero` assert the
fallback mean/rstd workspace as an absolute `16u`, but the backward workspace
is the sum of that term and a parallel reduction buffer whose presence depends
on the compute-unit count of the machine running the tests:

    reqd       = 4 * localSize * multiProcessorCount
    isParallel = innerSize <= reqd && innerSize * outerSize > reqd

For their shape that is true for one band of CU counts and false outside it, so
the expected value is only correct on some machines and both tests fail on the
rest. Test pools that mix hardware therefore pass or fail by runner draw.

Assert the difference between the same shape with and without mean/inv_variance
instead. The parallel term is identical for both graphs, so it cancels: the
difference is the fallback mean/rstd workspace alone on every machine, with no
CU-dependent expected value and no assumption about which branch was taken.
@BrianHarrisonAMD
BrianHarrisonAMD force-pushed the users/bharriso/layernorm-workspace-test-shape branch from e48f676 to b7ec1f8 Compare September 23, 2026 15:09
@BrianHarrisonAMD
BrianHarrisonAMD marked this pull request as ready for review September 23, 2026 15:15

@EwanC EwanC 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.

Thanks for tracking down this issue and coming up with a fix.

Patch makes sense to me, to rephrase in my own understanding: The plan needs a workspace in either of the cases a) no provided optional stats tensors b) parallel case which is determined based on device CUs. The test assumption was that the parallel case wouldn't be triggered and only the optional tensors would be used in workspace size calculation, but actually some of the devices stressed by CI did have a number of CUs that hit the parallel case and the extra size requirement threw off the hardcoded 16 workspace size value.

TLDR, LGTM

@BrianHarrisonAMD
BrianHarrisonAMD enabled auto-merge (squash) September 23, 2026 16:54

@adickin-amd adickin-amd 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.

LGTM

@BrianHarrisonAMD
BrianHarrisonAMD merged commit dd79e3c into develop Sep 24, 2026
92 checks passed
@BrianHarrisonAMD
BrianHarrisonAMD deleted the users/bharriso/layernorm-workspace-test-shape branch September 24, 2026 03:12
@assistant-librarian

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.

Layernorm bwd workspace tests pass or fail depending on the CU count of the test machine

3 participants