Skip to content

[AIROCMLIR-426] Translate rocMLIR conv layouts to MIOpen or skip#2422

Merged
bogdan-petkovic merged 4 commits into
developfrom
users/bpetkovi/2212-miopen-layout-translate-or-skip
Jul 16, 2026
Merged

[AIROCMLIR-426] Translate rocMLIR conv layouts to MIOpen or skip#2422
bogdan-petkovic merged 4 commits into
developfrom
users/bpetkovi/2212-miopen-layout-translate-or-skip

Conversation

@bogdan-petkovic

@bogdan-petkovic bogdan-petkovic commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Motivation

While running the MIOpen benchmark, every conv config fails with Invalid Layout Parameter Value because rocMLIR passes its own layout names (e.g. GNC01, NGC01) to MIOpenDriver, which only accepts NCHW/NHWC.
The failures were silently converted to NaN, so nightly CI has been green despite the MIOpen benchmarks never running.
Resolves https://github.com/ROCm/rocMLIR-internal/issues/2212 (AIROCMLIR-426).
Supersedes #2298.

Technical Details

  • Add rocmlir_layout_to_miopen() and conv_commandline_to_miopen_layouts(): translate -f/-I/-O to NCHW/NHWC only when the layout maps exactly (drop the group dim G — MIOpen conveys the group count via -g — and rename spatial dims 0->H, 1->W). Configs whose layouts have no faithful MIOpen equivalent, or where filter/input/output do not share a single layout, are skipped instead of benchmarked against a different layout.
  • MIOpen benchmark failures and output-parse failures now raise RuntimeError instead of silently returning NaN, so CI fails on real errors.
  • run_pipeline now returns the failing process's stderr, so the error message is no longer empty.
  • Add unit tests covering the translate/skip logic.

Over tier1-conv-configs: 1296/1363 configs are translated and benchmarked, 67 are skipped (all genuinely non-representable orderings or mixed NCHW/NHWC).

Test Plan

  • yapf/flake8 clean on changed files
  • pytest mlir/utils/performance/tests/test_perfRunner.py

Test Result

  • PR CI
  • Nigthly CI

Submission Checklist

@bogdan-petkovic bogdan-petkovic self-assigned this Jul 6, 2026
@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2422      +/-   ##
===========================================
+ Coverage    82.57%   82.70%   +0.13%     
===========================================
  Files          120      120              
  Lines        42852    42828      -24     
  Branches      7110     7106       -4     
===========================================
+ Hits         35381    35417      +36     
+ Misses        4815     4794      -21     
+ Partials      2656     2617      -39     
Flag Coverage Δ
gfx120x 82.56% <ø> (+0.04%) ⬆️
gfx950 82.53% <ø> (+0.19%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…2-miopen-layout-translate-or-skip

Co-authored-by: Cursor <[email protected]>

# Conflicts:
#	mlir/utils/performance/perfRunner.py
@bogdan-petkovic

bogdan-petkovic commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Ran this through nightly and compared against develop.
On develop, all 273 conv configs in the chunk were failing MIOpenDriver with Invalid Layout Parameter Value and getting swallowed into NaN, which is exactly why the stage stayed green while MIOpen never actually ran anything.
With this branch, 266 of them translate to NCHW and run for real. The 7 that genuinely can't be expressed as NCHW/NHWC (the G0NC1/01NGC orderings and the mixed NCHW+NHWC ones) get skipped explicitly instead of faking a comparison. No silent failures, and if MIOpen ever errors for real it now fails the stage instead of hiding it.

develop this PR
Invalid Layout errors 273/273 0
actually benchmarked 0 266
skipped (no NCHW/NHWC equiv) 0 7

Both boards green

@bogdan-petkovic
bogdan-petkovic marked this pull request as ready for review July 13, 2026 12:27
@bogdan-petkovic
bogdan-petkovic requested a review from causten as a code owner July 13, 2026 12:27
@bogdan-petkovic
bogdan-petkovic requested a review from Copilot July 13, 2026 12:28
@bogdan-petkovic bogdan-petkovic added the claude-review Trigger automated PR review by claude[bot]; auto-removed after the run. label Jul 13, 2026

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.

Pull request overview

This PR fixes MIOpen convolution benchmarking by translating rocMLIR-specific conv layout strings (e.g., GNC01, NGC01) into MIOpenDriver-supported NCHW/NHWC layouts when the mapping is exact, and skipping configs that cannot be represented fairly. It also tightens failure handling so MIOpen benchmark/parse failures raise errors instead of silently producing NaN, improving CI signal.

Changes:

  • Add rocmlir_layout_to_miopen() and conv_commandline_to_miopen_layouts() to translate -f/-I/-O layouts to NCHW/NHWC or skip non-representable/mixed-layout configs.
  • Make MIOpen benchmark failures and elapsed-time parse failures raise RuntimeError instead of returning NaN.
  • Add unit tests for layout translation and translate-or-skip behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
mlir/utils/performance/perfRunner.py Adds rocMLIR→MIOpen layout translation/skip logic and hard-fails on MIOpen benchmark/parse errors.
mlir/utils/performance/tests/test_perfRunner.py Adds unit tests covering layout mapping and commandline translate-or-skip behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mlir/utils/performance/perfRunner.py
Comment thread mlir/utils/performance/perfRunner.py

@rocmlir-pr-reviewer rocmlir-pr-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE -- submitted as COMMENT (automated reviews are advisory)  ·  Findings: 0 (0 Critical, 0 Major, 0 Minor)


Scope

Python perf-tooling change in mlir/utils/performance/perfRunner.py (+ tests). Adds rocmlir_layout_to_miopen() / conv_commandline_to_miopen_layouts() to translate rocMLIR conv layout names (e.g. GNC01) into MIOpen's NCHW/NHWC — dropping the group dim G, renaming spatial 0->H/1->W, treating output channel K as C — or skipping configs with no faithful MIOpen equivalent (unrepresentable ordering, or filter/input/output disagreeing on NCHW vs NHWC). Also replaces silent-NaN MIOpen benchmark/parse failures with RuntimeError so CI fails on real errors.

Findings

No blocking issues found.

Notes

  • Verified the translate-or-skip logic against the tensor letter conventions (FILTER_LAYOUT_MAP/INPUT_LAYOUT_MAP/OUTPUT_LAYOUT_MAP): filter uses N+C, output uses N+K, input uses N+C, so the K->C replacement never produces a double-C collision. The chained str.replace order is collision-free.
  • Confirmed the new error handling matches run_pipeline's (outs, ok) contract (if not noerr: → failure), and that run_pipeline already prints the failing stage's stderr, so the "see the MIOpenDriver error above" wording in the raised RuntimeError is accurate.
  • Tests cover positive translation (NCHW/NHWC), MIOpen pass-through, K-as-C, group conv (-g preserved), unrepresentable ordering, and mixed NCHW/NHWC skip — good positive/negative coverage for a helper of this kind.
  • Minor observation (not blocking, no action needed): conv_commandline_to_miopen_layouts returns the commandline unchanged if no -f/-I/-O flags are present (empty seen_layouts), but a conv config always carries these, so this defensive path is unreachable in practice.

CI status

No failing or cancelled checks. All functional checks are still pending/in-progress; detect passed. The auto-review pipeline's own review check being in-progress is expected and is not a CI failure.

@rocmlir-pr-reviewer rocmlir-pr-reviewer Bot removed the claude-review Trigger automated PR review by claude[bot]; auto-removed after the run. label Jul 13, 2026

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

When this gets all of the required approvals can you also create a PR porting this change over to rocmlirTriton?

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.

Why not make these helper functions members of ConvConfiguration similar to how benchmark_external is already setup?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put these right next to the other layout helpers on purpose. input_layouts, filter_layouts and the inverse ones are all plain module level functions too, and they're actually shared between ConvConfiguration and ConvGemmConfiguration, which is why they don't live inside a single class. benchmark_external is a classmethod because it genuinely needs cls and things like table_entry and from_command_line. These two new ones are just pure string helpers with no class state, so making them staticmethods would mostly add a class prefix while making them inconsistent with the helpers sitting right above them. I'm happy to move them into ConvConfiguration if you'd prefer, I just wanted to keep it consistent with what's already there

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.

Ah okay. I'm okay with leaving them next to the other layout helpers then.

Comment on lines +151 to +152
if len(seen_layouts) > 1:
return None

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really true ? can you double check ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double-checked directly with MIOpenDriver on a gfx942 GPU, same conv, only changing the layouts:

# all NCHW
$ MIOpenDriver conv -F 1 -f NCHW -I NCHW -O NCHW -n 1 -c 512 -H 32 -W 32 -k 512 -y 3 -x 3 -p 1 -q 1 -u 1 -v 1 -l 1 -j 1 -m conv -g 1 -t 1 -V 0 -t 1
Elapsed: 0.113675 ms
# all NHWC
$ MIOpenDriver conv -F 1 -f NHWC -I NHWC -O NHWC ...
Elapsed: 0.109906 ms
# mixed: -f NCHW -I NCHW -O NHWC
$ MIOpenDriver conv -F 1 -f NCHW -I NCHW -O NHWC ...
No suitable algorithm was found to execute the required convolution   (rc = 0x7)

Same config runs fine for all-NCHW and all-NHWC, but fails the moment the layouts are mixed, MIOpen has no solver for mixed filter/input/output layouts. That's why I skip those configs so if we translated and ran them, the fail-on-error path would turn the nightly red

@umangyadav umangyadav left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Address justin's comment before merging

@bogdan-petkovic
bogdan-petkovic merged commit 8ac7cef into develop Jul 16, 2026
10 of 17 checks passed
@bogdan-petkovic
bogdan-petkovic deleted the users/bpetkovi/2212-miopen-layout-translate-or-skip branch July 16, 2026 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants