[dhernandez0]

Motivation

Port of ROCm/rocmlirTriton#474 to rocMLIR

Technical Details

See Motivation.

Test Plan

PR CI.

Test Result

All tests pass.

Submission Checklist

• Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/GOVERNANCE.md#pull-requests.

[Copilot]

🟡 Changes recommended

Move preparation before all timed paths and exclude it from the GPU timeout deadline.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Ports benchmark-overhead fixes to rocMLIR by filtering HIP runtime kernels and eagerly preparing cache-flush artifacts.

Changes:

• Excludes internal ROCclr kernels from profiling metrics.
• Adds tests for filtering and NaN handling.
• Adds cache-flush artifact preparation before timing.

File summaries

File	Summary
mlir/utils/performance/tests/test_perfRunner.py	Tests profiler filtering behavior.
mlir/utils/performance/perfRunner.py	Filters internal ROCclr kernels from metrics.
mlir/tools/rocmlir-tuning-driver/rocmlir-tuning-driver.cpp	Prepares cache artifacts before estimates; setup remains timed on the legacy path and may consume the GPU timeout.
mlir/tools/rocmlir-tuning-driver/CacheFlush.h	Declares cache preparation APIs.
mlir/tools/rocmlir-tuning-driver/CacheFlush.cpp	Implements cache-flush artifact preparation.

Review details

Suppressed comments (1)

mlir/tools/rocmlir-tuning-driver/rocmlir-tuning-driver.cpp:761

• gpuRunDeadline is created before this eager preparation (at rocmlir-tuning-driver.cpp:662), and synchronizeStreamWithTimeout treats it as an absolute deadline. The new ~100 ms hiprtc/hipMalloc setup therefore consumes the user’s --gpu-run-timeout, even though the option and the nearby comment say compilation/setup is excluded; with a short timeout this can terminate a valid run early. Reset/create the deadline after prepareCacheFlushArtifacts (or otherwise exclude this setup interval) before starting the estimate.

    if (failed(prepareCacheFlushArtifacts(params.flushLastLevelCache)))
      return failure();

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

---

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

[rocmlir-pr-reviewer]

This eager prepare only covers the --triton-benchmark-mode branch, but the legacy default path has the same defect. In measureSmallKernel the CPU timer starts at line 470 and the first flushInstructionCache/flushCache happens at lines 475-478, i.e. inside the timed region — so on the very first config of a tuning run the hiprtc compile plus the (up to 256 MiB with --flush-last-level-cache) hipMalloc is charged to smallKernelCpuMs. Since legacy mode is the default and tuning mode is exactly the case where benchmarkMode is false and the flushes do run, that first perf config gets a badly inflated time and can be ranked out even when it is the best one. Suggest hoisting the call above the if (!params.tritonBenchmarkMode) at line 670 so both paths get the artifacts built up front; if you want to avoid the large allocation in the legacy path that never flushes, gate it on params.tritonBenchmarkMode || params.benchmarkConfig.empty() instead of duplicating the call. (Checklist: Major — "DRY/YAGNI/KISS violations: redundant code" / incomplete fix across equivalent code paths.)

[bogdan-petkovic]

@dhernandez0 I agree with this one, please take a look

[rocmlir-pr-reviewer]

Verdict: COMMENT  ·  Findings: 1 (0 Critical, 1 Major, 0 Minor)

---

Scope

Two independent fixes that both keep HIP-runtime overhead out of benchmark numbers:

1. Tuning driver (C++). CacheFlush.cpp gains a prepare() entry point (prepareCacheFlushArtifacts) that eagerly does the two lazily-created, expensive one-time setup steps — the hiprtc compile of flush_icache_kernel and the cache-sized hipMalloc — and rocmlir-tuning-driver.cpp:760 calls it before the Triton-style estimate loop starts recording events, so that ~100 ms stall no longer inflates estimateMs and collapse every derived iteration count to its floor.
2. perfRunner (Python). New is_rocclr_internal_kernel() filters __amd_rocclr_* dispatches (the ROCclr blit shader that services the host harness's hipMemcpy, etc.) out of both get_nanoseconds() and get_bank_conflict(), and both now return np.nan rather than 0 / a div-by-zero when nothing of ours is left.

Findings

• mlir/tools/rocmlir-tuning-driver/rocmlir-tuning-driver.cpp:760 (Major) — the eager prepare only runs on the --triton-benchmark-mode branch; the legacy default path has the same problem in measureSmallKernel, where the CPU timer starts at line 470 before the first flushCache() at line 478.

Notes

Spot-checks that came out clean:

• The Name / Kernel_Name column split in the docstring at perfRunner.py:343 matches how the files are actually produced: get_nanoseconds parses results_kernel_stats.csv from rocprofv3 --kernel-trace --stats --output-format=csv (perfRunner.py:2165), whose stats report uses Name; the counter-collection report under pmc_1/ uses Kernel_Name. The two headers are disjoint, so the first-match loop cannot pick the wrong column.
• np.nan is safe downstream — compute_tflops divides by the nanoseconds value and the result goes through df.fillna(np.nan) before to_csv, so NaN propagates as "N/A" rather than reading as infinite TFlops (which a 0 return would have done).
• Preserving the sum over rows in get_nanoseconds is correct and now documented; a strided backward-data convolution legitimately emits several kernels for one op.
• measureLargeKernel and measureKernel already flush outside their event windows, so they were never affected by the lazy-init cost.
• New Python tests are well-targeted (exclusion, all-internal, missing-column fallback) and stay under the 100-column yapf/flake8 limit; os and tempfile are still used elsewhere in the test file, so the tmp_path migration leaves no unused imports.

Pre-existing issues (out of scope)

• get_bank_conflict returns the string "NaN" when the file is missing (perfRunner.py:391) but np.nan on the other two no-data paths. The PR's new test_missing_file_returns_nan_string pins that string behaviour; worth unifying on np.nan in a follow-up so callers don't have to handle two types.
• The csv_file.close() calls inside with open(...) blocks (perfRunner.py:364, :403) are redundant.

CI status

All non-self checks (Python format and lint checks, Python performance script tests, C/C++ premerge checks) were still in progress at review time; nothing red yet.