Skip to content

Tuning quality-of-life improvements - #2451

Open
mirza-halilcevic wants to merge 3 commits into
developfrom
tuning-qol-improvements
Open

Tuning quality-of-life improvements#2451
mirza-halilcevic wants to merge 3 commits into
developfrom
tuning-qol-improvements

Conversation

@mirza-halilcevic

@mirza-halilcevic mirza-halilcevic commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Motivation

Assorted tuning quality-of-life improvements.

Technical Details

  • Use llvm::scope_exit instead of the deprecated llvm::make_scope_exit in rocmlir-tuning-driver, silencing the compiler warnings.
  • Report measurements and stats in nanoseconds. The returned timing was already converted, so the JSON printed next to it described the same run in a different unit.
  • Name the offending perf config when compilation crashes. A fatal error handler and a signal handler print the config held by the crashing compilation thread together with the --benchmark-config flag that reproduces it, instead of leaving an unattributable stack trace.
  • Write per-config stats rather than raw per-iteration measurements to the debug file. The measurement arrays grow with the iteration count and come to dominate the file, while min/max/median/stddev/cv carry what we read back from it.
  • Accept the legacy CU count on WGP-mode chips. Results and state written before the count came from HIP hold twice the current value on RDNA, so they looked like another device and were tuned again from scratch. Such entries are now reused, and a legacy state context is merged into the current one and dropped from the file.
  • Default the output path to the --configs-file path with a '.tsv' extension, falling back to tuning_results_local.tsv when there is nothing to derive it from (--config, --test-dir or stdin).
  • Move argument parsing into its own section and fold the --gpu-run-timeout check in with the rest of the validation in TuningArgumentParser.

Submission Checklist

- Use llvm::scope_exit instead of deprecated llvm::make_scope_exit
- Report measurements and stats in nanoseconds, like the final result
- Name the offending perf config when compilation crashes
- Write stats instead of per-iteration measurements to the debug file
- Accept the legacy doubled CU count recorded on WGP-mode chips
- Default the output path to the configs file name with a .tsv suffix
@mirza-halilcevic
mirza-halilcevic requested a balanced review from Copilot August 17, 2026 11:26
@mirza-halilcevic mirza-halilcevic added the claude-review Trigger automated PR review by claude[bot]; auto-removed after the run. label Aug 17, 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

Improves tuning diagnostics, output consistency, legacy device-data reuse, and argument handling.

Changes:

  • Reports timing statistics in nanoseconds and adds crashing perf-config diagnostics.
  • Reuses and migrates legacy WGP CU-count results/state.
  • Derives output paths from config files and consolidates argument validation.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
mlir/utils/performance/tuningRunner.py Updates state migration, statistics, output naming, and parsing.
mlir/utils/performance/tests/test_tuningRunner.py Tests legacy CU handling and output defaults.
mlir/utils/performance/perfRunner.py Adds WGP-mode detection.
mlir/utils/performance/tests/test_perfRunner.py Tests WGP-mode detection.
mlir/tools/rocmlir-tuning-driver/rocmlir-tuning-driver.cpp Updates timing units, scope guards, and crash diagnostics.
mlir/test/perf-scripts/runtime/tuningRunner-gemm.py Updates debug-output checks for statistics.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread mlir/utils/performance/tuningRunner.py Outdated
@rocmlir-pr-reviewer rocmlir-pr-reviewer Bot removed the claude-review Trigger automated PR review by claude[bot]; auto-removed after the run. label Aug 17, 2026
@mirza-halilcevic mirza-halilcevic added the claude-review Trigger automated PR review by claude[bot]; auto-removed after the run. label Aug 17, 2026
Comment thread mlir/utils/performance/tuningRunner.py Outdated
Comment thread mlir/tools/rocmlir-tuning-driver/rocmlir-tuning-driver.cpp Outdated

@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: COMMENT  ·  Findings: 2 (0 Critical, 1 Major, 1 Minor)


Scope

Tuning quality-of-life changes across rocmlir-tuning-driver.cpp, tuningRunner.py, perfRunner.py, their pytest suites, and one lit test: swap deprecated llvm::make_scope_exit for the llvm::scope_exit constructor, report measurements/stats in nanoseconds, name the offending perf config on a compilation crash, write per-config stats instead of raw per-iteration arrays to the .debug file, accept the legacy (doubled) CU count on WGP-mode chips, derive the default --output from --configs-file, and consolidate argument validation into TuningArgumentParser.

Findings

  • mlir/utils/performance/tuningRunner.py:1969default_output_path() returns the input path unchanged when --configs-file already ends in .tsv; the writer then appends result rows to the user's own configs file. (Copilot raised the same point; repeating it because it is genuine and unaddressed.)
  • mlir/tools/rocmlir-tuning-driver/rocmlir-tuning-driver.cpp:722llvm::errs() is used from both a fatal-error handler and a signal handler, which is the one context LLVM deliberately avoids raw_ostream in.

Notes

  • Verified the scope_exit swap: external/llvm-project/llvm/include/llvm/ADT/ScopeExit.h:47 supplies the CTAD deduction guide and line 56 marks make_scope_exit LLVM_DEPRECATED, so the mechanical change is correct and the [[nodiscard]] class is still bound to a named variable everywhere.
  • The ms→ns move is consistent: measureSmallKernel pushes exactly one measurement, so the small-kernel --show-stats path still emits exactly one JSON field and find_best_perfconfig's len(parts) == 3 test continues to hold. The lifetime comment on compilingConfig also checks out — configs is declared before threads/threadCleanup in the same iteration scope, so it outlives the joins on every exit path.
  • --show-all-measurements now has no in-repo caller. Its cl::desc (and --show-stats') still do not state a unit, which is now nanoseconds; since unit clarity is the point of this PR, a one-line desc update on both options would be worth folding in.
  • matches_current_num_cu() cannot distinguish a legacy record of this device from a genuine record of a same-gfx SKU with exactly twice the WGP count. No such pair looks likely today, but if you want to bound it, preferring exact-CU rows and only falling back to doubled rows for test vectors with no exact match would narrow the window.

CI status

No non-self check is failing. The review FAILURE entry is this auto-review pipeline's own check. Jenkins (Build and Test, MIGraphX, Parameter sweeps, Code coverage) is still pending; the Python format/lint, Python performance script tests, and C/C++ premerge checks have all passed.

@rocmlir-pr-reviewer rocmlir-pr-reviewer Bot removed the claude-review Trigger automated PR review by claude[bot]; auto-removed after the run. label Aug 17, 2026
@mirza-halilcevic mirza-halilcevic added the claude-review Trigger automated PR review by claude[bot]; auto-removed after the run. label Aug 18, 2026
@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2451      +/-   ##
===========================================
+ Coverage    82.57%   83.49%   +0.93%     
===========================================
  Files          120      121       +1     
  Lines        42852    43182     +330     
  Branches      7110     7181      +71     
===========================================
+ Hits         35381    36053     +672     
+ Misses        4815     4520     -295     
+ Partials      2656     2609      -47     
Flag Coverage Δ
gfx120x 83.38% <ø> (+0.85%) ⬆️
gfx950 83.40% <ø> (+1.05%) ⬆️

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

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

@rocmlir-pr-reviewer rocmlir-pr-reviewer Bot removed the claude-review Trigger automated PR review by claude[bot]; auto-removed after the run. label Aug 18, 2026
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.

2 participants