Repository navigation
feat(miopen): Add gfx120x support to Winograd Rage - #6439
Conversation
a29097b to
49d14fb
Compare
3ca253a to
154b76b
Compare
|
@scerzh shapes from this comment (see any of attached log files) can be used for testing. |
…rad-rage-add-gfx12
…rad-rage-add-gfx12
SreecharanGundaboluAMD
left a comment
There was a problem hiding this comment.
Please review all the changes where you are doing device checks for "gfx12".
…rad-rage-add-gfx12
…dd FP16 NHWC tests Re-register the conv::TransposedConvWinoRageRxS<2, 3> solver (NHWC transposing wrapper around ConvWinoRageRxS) that was disabled when the Winograd Rage integration was reverted. The transpose wrapper inherits the forward solver's applicability, which is FP16-only on this branch, so it cannot reach the fp32 kernels implicated in the revert. Restore the FP16 NHWC unit tests for the transposed solver, including the g=1 and degenerate spatial-dim regression cases for the GetGroupConvLayout NHWC WrW fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
❌ PR Check — Action Required
📖 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 |
false positive, tests are added. |
|
@JonathanLichtnerAMD could you please review and help to merge? |
|
Hi All, The therock-pr-bot is not blocking now you from merging this ,currently this is only showing the issues and raising the Not ready to review flag.We are collecting the requirements across and will be providing enhancements in the upcoming releases. |
There was a problem hiding this comment.
We have some MIOpenDriver out-of-bounds access checks that can be used:
- env variable: MIOPEN_DEBUG_CHECK_SUB_BUFFER_OOB_MEMORY_ACCESS=2
- MIOpenDriver option: --gpubuffer_check=2
Both are needed, and both check different buffers. (You can also run with these set to 1, but we've never seen an issue before the start of the buffer--it has always been an illegal access after the buffer.) Note that ASAN would not help with the Winograd kernels, since these are assembly kernels.
Can we run a wide set of shapes using the above env variable and flag just to make sure we don't hit any new issues?
|
I asked claude to spit out some suggested shapes to run:
|
|
And just because it might be missed in the above comment, we can also run the unit tests with MIOPEN_DEBUG_CHECK_SUB_BUFFER_OOB_MEMORY_ACCESS to provide extra coverage. |
|
on gfx1201:
|
JonathanLichtnerAMD
left a comment
There was a problem hiding this comment.
LGTM. Thanks for doing the extra testing. I've created a jira issue for the unrelated failure
|
on gfx942: Same OOB / guard-band test plan as the gfx1201 run above, executed on gfx942 against this build's Every driver line was run twice — once with
(Using
(
Note: unlike gfx1201, these pass on gfx942 with
Summary: all groups pass under both page-guard modes with no GPU memory faults; the unit suite (including the new transposed Rage kernels) is green (122 passed / 0 failed); correctness verified against the GPU reference for fwd / bwd-data / wrw. The #6196-class overrun does not reproduce on gfx942. |
JIRA ID: ALMIOPEN-2364 ## Motivation ROCm 7.15 has been renamed to ROCm 10.0, and MIOpen 3.6.0 is the corresponding release. `projects/miopen/CHANGELOG.md` needs a new version section documenting what changed for downstream users, release-notes tooling, and packagers between MIOpen 3.5.2 (ROCm 7.14.0) and MIOpen 3.6.0 (ROCm 10.0.0), consistent with the changelog's existing per-release format. ## Technical Details - Inserted a new `## MIOpen 3.6.0 for ROCm 10.0.0` section at the top of `projects/miopen/CHANGELOG.md`, directly above the existing `## MIOpen 3.5.2 for ROCm 7.14.0` section, using the same `### Added` / `### Changed` / `### Resolved Issues` / `### Removed` subheading structure as every prior release section in the file. - Entries were compiled by diffing `origin/release/therock-10.0` against `origin/release/therock-7.14` for `projects/miopen` and hand-selecting changelog-worthy commits, excluding: test-only commits, CI-only commits, build/version-bump commits, doc-only commits, reverted commit/revert pairs, and commits already surfaced in the 3.5.2/7.14.0 section via cherry-pick. - Resulting entries added: - **Added:** gfx950 (MI350X/MI355X) 7x7 depthwise forward and backward-data convolution support, fp16/bf16 (#9168) — fixes a slow fallback-to-naive-kernel regression in ConvNeXt-style depthwise convolutions. - **Changed:** - Restored gfx12x support in the Winograd Rage solver (#6439), recovering performance lost when earlier gfx12 support was reverted. - Refreshed the gfx1100, gfx1102, and gfx1201 (Navi) SystemDBs (#9529) with updated tuned find/perf-database entries. - Refreshed the gfx950 SystemDB (#9111) with additional tuned entries. - **Resolved Issues:** - Fixed an off-by-stride indexing bug in BatchNorm backward `CalcStats` mean/variance remainder loop causing a ~1% systematic bias in NCHW batchnorm backward results (#9031). - Fixed a tensor-op integer overflow on gfx1250 for large allocations that could cause memory access faults (#9049). - Fixed naive convolution solvers silently failing to launch (and leaving a stale HIP error visible after Find returned success) when a global work size reached 2^32 or more work-items; such launches are now rejected up front (#10188). - **Removed:** the OpenCL (OCL) backend (#8433); MIOpen now supports the HIP backend only. - No code, build, or test files were touched — this is a pure documentation addition; the underlying functional changes were already merged to `release/therock-10.0` in their own PRs (referenced above) and are only being documented here. ## Risk Assessment Single file (`projects/miopen/CHANGELOG.md`), 17 lines added, 0 removed, no code or build-system changes. **Risk Level: 🟢 Low** ### Impacted Components - `projects/miopen/CHANGELOG.md` only — no source, kernel, solver, build, or test files are touched. ### Potential Side Effects - None functionally. The only observable effect is the new changelog text itself; no build artifacts, APIs, or runtime behavior change. - Cosmetic risk only: if any release-notes/packaging tooling parses this file programmatically, it should be checked for compatibility with the new section, though the added section strictly follows the existing heading/bullet conventions used by all prior sections in the file. ### Mitigation Steps - Followed the exact existing section format (`##` version header, `###` category subheadings, `* ` bullet entries, `[Component]` prefix tags) used by the adjacent 3.5.2/7.14.0 and 3.5.1/7.13.0 sections, minimizing formatting risk. - Each entry was cross-referenced against a specific merged PR number (#9168, #6439, #9529, #9111, #9031, #9049, #10188, #8433) rather than paraphrased from memory, so reviewers can verify each line against its source commit. ## Test Plan No functional tests apply — this is a documentation-only change to a Markdown changelog file. Verification consisted of manually reviewing the diff against the existing file's structure and confirming the new section's heading hierarchy and bullet formatting match every other version section in `CHANGELOG.md`. ## Test Result - `git diff origin/develop...HEAD --stat` confirms the change is scoped to exactly one file (`projects/miopen/CHANGELOG.md`, +17/-0). - Visual/manual review confirms the new `## MIOpen 3.6.0 for ROCm 10.0.0` section renders as valid Markdown consistent with surrounding sections. No build or CI run is applicable to a docs-only change. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
JIRA ID : ALMIOPEN-1853
Motivation
Previous Rage update #2429 was reverted by #6196 due to memory faults found on several models. Support for gfx12 was lost in Rage solver leading to a performance drop on gfx12.
Technical Details
The reverted PR was split into dedicated steps. This step brings gfx12 support back.
Test Plan
madengineTestsTest Result
madengineTestsmiopen_gtest --gtest_filter=*UnitTestConvSolverWinoRage2x3*The average gain on certain shapes is around 44%
Submission Checklist
madengineTests