Skip to content

fix(miopen): hold ConvHipConv out of the perf-config picker - #13337

Merged
jdcampbe merged 2 commits into
developfrom
users/jascampb/disable-convhipconv-pcfg-picker
Oct 8, 2026
Merged

jdcampbe merged 2 commits into
developfrom
users/jascampb/disable-convhipconv-pcfg-picker

Conversation

@jdcampbe

@jdcampbe jdcampbe commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Holds ConvHipConv out of the LGBM perf-config (layer-2) picker until its model is retrained, so it falls back to the solver's own default config.

ConvHipConv's pcfg model was trained on an older hipconv config vocabulary and over-selects a slow config (direct[128,1,16,16]): 1.2–1.4x slower than the solver default on large shapes (~0.89x OOTB geomean on the regression set), helping only some small shapes. hipconv's config descriptors and kernel set have since changed and are now version-tagged, so the model must be retrained on current-hipconv data before it can be trusted.

JIRA ID : ALMIOPEN-2494

Change

One guard at the top of PickConfig (lgbm_pcfg_pick.cpp): return no candidates for ConvHipConv, next to the existing two-tower deferral. The other 13 solvers are unaffected.

Gated in code, not by dropping the section from lgbm_pcfg.bin, so:

  • re-enabling after a retrain is a one-line revert, and
  • it holds regardless of whether the bundle is later regenerated.

Why a standalone PR off develop

This is independent of the two in-flight ConvHipConv efforts and composes with either:

  • The perf-config serialization fix (PerformanceConfigConvHipConv reading its comma-containing descriptor whole) — on today's develop the broken read already neuters ConvHipConv pcfg picks; this guard makes "held out" explicit so it stays out once serialization is fixed.
  • The hipconv v0.4.0 bump, which version-tags descriptors (old-format pcfg descriptors are rejected anyway); this guard is a belt-and-suspenders that survives a bundle regeneration.

Test plan

  • lgbm_pcfg_pick.cpp compiles clean (HIPNOGPU per-TU).
  • On a gfx950/gfx1250 build with MIOPEN_LOG_LEVEL=6, a ConvHipConv-eligible conv shows lgbm_pcfg: ConvHipConv held out of the picker ... and the solver runs its default config (no lgbm_pcfg: ConvHipConv ranked ... line).

Follow-up

Re-collect ConvHipConv tuning data against a current hipconv (new descriptor format + kernel set), retrain + validate, then revert this guard.

ConvHipConv's perf-config model was trained on an older hipconv config
vocabulary and over-selects a slow config (direct[128,1,16,16]), ~0.89x
OOTB geomean on the regression set. hipconv's config descriptors and
kernel set have since changed and are now version-tagged, so the model
needs retraining on current-hipconv data before it can be trusted.

Return no candidates for ConvHipConv in the pcfg picker so it falls back
to the solver's own default. Gated in code rather than by dropping the
section from lgbm_pcfg.bin, so re-enabling after a retrain is a one-line
revert.

Co-Authored-By: Claude <noreply@anthropic.com>
@therock-pr-bot

therock-pr-bot Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

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/miopen/src/conv/heuristics/lgbm_pcfg_pick.cpp; no test file found
🔎 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

therock-pr-bot Bot commented Oct 8, 2026

Copy link
Copy Markdown

Pre-commit check failed

⛔ pre-commit failed

Please run locally:

  • python -m pip install pre-commit
  • pre-commit install
  • pre-commit run --all-files --show-diff-on-failure

This repo uses .pre-commit-config.yaml.

Collapse HeldOutOfPicker onto one line per the project clang-format.

Co-Authored-By: Claude <noreply@anthropic.com>
@therock-pr-bot

therock-pr-bot Bot commented Oct 8, 2026

Copy link
Copy Markdown

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

@jdcampbe
jdcampbe enabled auto-merge (squash) October 8, 2026 20:19
@jdcampbe
jdcampbe merged commit cf0ac8d into develop Oct 8, 2026
97 checks passed
@jdcampbe
jdcampbe deleted the users/jascampb/disable-convhipconv-pcfg-picker branch October 8, 2026 22:22
@assistant-librarian

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

mattwill-amd pushed a commit that referenced this pull request Oct 14, 2026
JIRA ID: ALMIOPEN-2913

## Motivation

Only 5 of the 30 commits that landed against `projects/miopen` since the
3.6.2 version bump (`2d20a8f775e`, #12211) were reflected in
`CHANGELOG.md`. The remaining 25 were reviewed and the 15
externally-visible ones documented, so the ROCm 10.2.0 section
accurately reflects what shipped.

14 entries added (two of the fixes share one entry), plus a
duplicate-heading fix.

### Added
* [Conv] NHWC (channel-last) support in the GEMM convolution solvers,
with new im2col/col2im kernels (#8693)
* [Conv] Cross-architecture AI heuristics for immediate mode
(`MIOPEN_FIND_MODE=2`), extending data-driven selection to Navi (#9680)
* [Conv] Missing 3x3 stride-1 pad-1 tile-28 CK depthwise backward-data
instance — ALMIOPEN-2423 (#11271)

### Changed
* [Conv] `ConvHipConv` held out of the AI perf-config picker until its
model is retrained — ALMIOPEN-2494 (#13337)
* [Conv] CK byte-span gate lifted for forward and backward-data, now
that CK fixes the 32-bit offset overflow itself — ALMIOPEN-2645 (#12396)

### Optimized
* [Conv] `ConvDirectNaiveConv`: per-direction kernel files, float
accumulators, spatial tiling, bfloat16 path — ALMIOPEN-2485/2486 (#7529)
* Kernel cache reuses cached SQLite connections instead of reopening per
load/store — ALMIOPEN-2598 (#11933)

### Resolved Issues
* [Conv] Incorrect backward-weights results from the gfx950 direct
kernel at `waves_q == 1` (missing cross-lane ordering on LDS staging
reuse) — ROCM-31508 (#12406)
* [Conv] Up to 8x slower hipconv `direct`/`direct_l1`/`direct_wgrad`
under ROCm 10.1's full AMDGCN LTO default; fixed with `-fno-offload-lto`
— ROCM-31334 (#12328)
* [Conv] gfx942 multi-node training throughput regression from backward
GEMM on small-batch 1x1 NHWC shapes — ROCM-31155 (#13232)
* [Conv] Invalid CK perf-config entries in the gfx1201 and gfx1100
system databases — ROCM-29026 (#12088, #12117)
* [BatchNorm] JIT compilation failures on non-GFX9 targets; Welford DPP
assembly restricted to GFX9 — ALMIOPEN-2838 (#12889)
* Out-of-bounds write in `miopenLoadSolution` when a serialized solution
carries more than three work dimensions — ROCM-31082 (#12242)
* `uint64_t` typedef conflict between `miopen_cstdint.hpp` and system
`<stdint.h>` on LP64 Linux (#12623)

## Technical Details

Single-file change to `projects/miopen/CHANGELOG.md`.

The 10.2.0 section had **two** `### Resolved Issues` headings. The two
entries under the trailing one (`ConvHipConv` gfx1250 crash/LDS race,
and the perf-db reload config loss) were merged into the first, and the
trailing heading removed — this accounts for the 2 removed bullets in
the diff. Section order is now `Resolved Issues → Added → Changed →
Optimized → Removed`, matching the 3.6.1 section. `Optimized` is new to
this release section.

Already documented, so untouched: #12874 (reduce int32 rejection),
#12000 / #13016 / #13213 (hipconv v0.3.1 / v0.3.2 / v0.4.0), #11761
(gfx803 removal).

## Notes

Deliberately excluded as not externally visible (10 commits):

* **hipDNN convolution forwarding (#12276)** —
`MIOPEN_ENABLE_HIPDNN_WRAPPER` defaults to `OFF`
(`projects/miopen/CMakeLists.txt:162`), so no shipped build compiles the
forwarding code and the runtime `MIOPEN_HIPDNN_FORWARDING` flag has
nothing to enable.
* **Test-only**: #13289 (gfx1250 depthwise test applicability,
ALMIOPEN-2841), #11610 (hipDNN parity harness), #12660 (Jenkins status
post).
* **Internal kernel-math cleanup, generated ISA unchanged**: #13312
(drop direct `__ocml` calls), #13190 (remove broken single-argument
`__half` overloads).
* **Add/revert pairs, net no change**: #12466 + #13066 (gfx1101 Navi
system DBs), #12496 + #12800 (gfx1250 in the dev image arch lists).

## Risk Assessment

Documentation-only change to a single Markdown file; no code, build, or
runtime impact.

**Risk Level: 🟢 Low**

## Test Plan

None required — Markdown-only. Verified that the entry topics are not
already documented elsewhere in the file, that no content was lost when
merging the duplicate heading, and that every new entry maps to a commit
in `2d20a8f775e..origin/develop -- projects/miopen`.

## Test Result

16 bullets added, 2 relocated — 14 net-new entries covering 15 commits.
Duplicate heading removed; section ordering matches 3.6.1.

Co-authored-by: Claude <noreply@anthropic.com>
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.

2 participants