Repository navigation
[AIROCMLIR-1318] Fix perf benchmark tooling: hipBLASLt/CK baselines, fusion benchmarks, fusion report - #545
Conversation
| rocmlir_gen_args = [ | ||
| '-ph', '-fut=' + fut_name + '_wrapper', '--perf_config=' + best_perf, '-' | ||
| ] | ||
| rocmlir_gen_args = ['-ph', '-fut=' + fut_name, '--perf_config=' + best_perf, '-'] |
There was a problem hiding this comment.
rocMLIR back-port check: every file this PR touches is under mlir/utils/performance/, which docs/PR_REVIEW_CHECKLIST.md ("rocMLIR back-port check -- Likely needs a back-port") lists as shared with ROCm/rocMLIR. The shared paths here are perfRunner.py, reportUtils.py, common/benchmarkUtils.{h,cpp}, hipblaslt-benchmark-driver/hipblaslt-benchmark-driver.cpp and ck-benchmark-driver/ck-gemm-benchmark-driver.cpp. The description's Dependencies section only refers to rocmlirTriton PR #361, so none of options (a)/(b)/(c) is satisfied. Please add either a link to a parallel rocMLIR PR, or a one-line note per fix explaining why it does not apply upstream -- fix 2 plausibly does not (the _wrapper removal came from this tree's #76), but the -transO parser fix and the fusion-report index fix look like they would apply to rocMLIR unchanged.
|
|
||
| CONV_CSV = """\ | ||
| Direction,DataType,Chip,numCU,numChiplets,FilterLayout,InputLayout,OutputLayout,N,C,H,W,K,Y,X,DilationH,DilationW,StrideH,StrideW,PaddingH,PaddingW,PerfConfig,LDSBankConflict,Fusion TFlops,MLIR TFlops,Fusion/MLIR,FileName | ||
| fwd,i8,gfx942,304,8,gkc01,ngc01,ngk01,1,128,56,56,128,3,3,1,1,2,2,1,1,,NaN,0.712234,1.522267,0.467877,mixr-resnet-fusion-case-1-quantization.mlir |
There was a problem hiding this comment.
The fixture's FileName column holds bare basenames, but benchmark_fusion_kernels() populates it from glob.glob(test_dir + '/*.mlir') (perfRunner.py:3048, assigned at :3101 and :3119), so the real CSV always carries a test_dir-prefixed path. The docstring claims these rows "are taken from a gfx942 run over resnet50-e2e and bert-torch-tosa-e2e", which is then inaccurate. The test still exercises the duplicate-index failure either way, but please either use the path form perfRunner actually writes so the fixture matches production data, or drop the provenance claim from the docstring.
| import unittest | ||
| from pathlib import Path | ||
|
|
||
| PERF_DIR = Path(__file__).resolve().parents[2] / "utils" / "performance" |
There was a problem hiding this comment.
This imports createFusionPerformanceReports and reportUtils from the source tree, while the sibling perfRunner-test.py:24-32 in the same directory deliberately resolves the script via shutil.which() and documents why (the deployed copies under ROCMLIR_BIN_DIR are what ci-performance-scripts ships and what the Jenkins perf stage runs). Neither module needs the compiled amd_arch_db binding, so the source-tree import works today, but it leaves two opposing conventions side by side and means this test would still pass if a script were dropped from PERFORMANCE_SCRIPTS in mlir/utils/performance/CMakeLists.txt. Prefer resolving the path the same way the sibling test does, or add a short comment stating why this file intentionally differs.
There was a problem hiding this comment.
Verdict: COMMENT · Findings: 3 (0 Critical, 1 Major, 2 Minor)
Scope
Three independent fixes to the performance tooling, one commit each: (1) -transO= is now accepted by the shared benchmark-driver argument parser (common/benchmarkUtils.{h,cpp}), with the hipBLASLt and CK drivers rejecting true explicitly; (2) benchmark_fusion_kernels() times the fused kernel under -fut=<name> instead of the no-longer-generated <name>_wrapper; (3) clean_data_for_humans() adds FileName to the row index so two files fusing different ops around the same problem no longer collide into a non-unique index. Two new pure-Python lit tests cover (2) and (3).
Findings
mlir/utils/performance/perfRunner.py:3106— every changed source file lives undermlir/utils/performance/, which the checklist lists as shared withROCm/rocMLIR; the PR description has no back-port note (Major).mlir/test/perf-scripts/fusion-performance-report.py:34— the fixture'sFileNamevalues are bare basenames, butbenchmark_fusion_kernels()writes theglobpath (Minor).mlir/test/perf-scripts/fusion-performance-report.py:23— imports the scripts from the source tree while the sibling test in the same directory deliberately imports fromPATH(Minor).
Notes
Spot-checks that came out clean:
- The
-transO=branch inparseCommandLine()matches the existing-transA=/-transB=idiom exactly, andgenerate_problem_commandline()emits-transO=True/-transO=False, whichatob()handles.printUsage()andprintProblem()were both updated. run_fusion_kernel()already invokesrocmlir-gen -fut <fut_name> --clone-harness, so the new-fut=<name>in the secondrocmlir-genstage is consistent with the harness it consumes.FileNameis only ever written bybenchmark_fusion_kernels(), so thereportUtilsindex change cannot reach the MLIR-vs-hipBLASLt/CK/MIOpen or regression reports. It is appended afterPerfConfig, but nothing consumes the returnedindex_colspositionally (perfRegressionReport.pyslices the raw*_TEST_PARAMETERSlists, notindex_cols).- Both new tests genuinely fail without their fix:
assertInover the argv list is an exact-element match, so_wrapperwould be caught, and the duplicate-index rows make pandas'Stylerraise.
Out of scope: the -transO handling in the CK/hipBLASLt drivers has no automated coverage because those drivers are not built in PR CI, as the PR description acknowledges.
CI status
No failing or cancelled checks. Jenkins, Build and Test, MIGraphX and Code coverage are still pending; py-checks passed.
…318-tooling-fixes
Motivation
While enabling the nightly "Benchmark and Report Performance" stage in rocmlirTriton (AIROCMLIR-901), I ran the stage's perfRunner.py and report commands locally on gfx942 and hit three issues in the perf tooling. The stage hasn't been running in rocmlirTriton so far, so nothing exercised these paths. All three are present in current develop:
-transOto the GEMM problem config, and perfRunner passes it tohipblaslt-benchmark-driverandck-gemm-benchmark-driveras well. Their shared argument parser doesn't recognize it yet and exits withInvalid argument!. perfRunner records that as NaN and carries on, so the "MLIR vs hipBLASLt" and "MLIR vs CK" reports would have no baseline.-fut=<name>_wrapper, but since [AIROCMLIR-548] Fixclone-harnessbehavior in rocmlir-gen #76rocmlir-gen --clone-harnessno longer creates a_wrapperfunction. With [AIROCMLIR-899] Enable weekly CI stages (parameter sweeps + tuning) #361's-archfix, all 34 files in resnet50-e2e and bert-torch-tosa-e2e hit the"does -fut point to the wrong function?"assertion, and the Fusion TFlops column stays empty.mixr-resnet-fusion-case-1-quantization/-case-1-int8,bert_part_0/bert_part_5). Each gets its own row (our tuning keys include the fused operations), but the report tells rows apart only by the conv/GEMM parameters, so it sees duplicates and fails ("non-unique index"). This happens whether or not the fusion runs succeed, so with the nightly stage on, "Create performance reports" would fail every night.Technical Details
One commit per fix.
Accept
-transOin the benchmark drivers: the shared parser (common/benchmarkUtils.cpp) now accepts-transO=the same way as-transA=/-transB=. The hipBLASLt and CK drivers fail with a clear error if it'strue, since neither driver handles transposed output (no tier1 gemm config uses-transO true).Use
-fut=<name>for fused kernels: one-line change inbenchmark_fusion_kernels(), matching the RUN lines of the fusion tests. It's also covered by a new test inperfRunner-test.py.Add
FileNameto the fusion report index:clean_data_for_humans()addsFileNameto the row index when the column exists, and only fusion results have it. The colliding rows stay separate, since they're different kernels. The CSVs and all other reports are unchanged; in the fusion HTML reports,FileNamejust moves into the row labels. This is covered by a new testperf-scripts/fusion-performance-report.pyDependencies
-targetstorocmlir-driver, soperfRunner --op=fusionfails before it reaches the step this PR fixes ([AIROCMLIR-899] Enable weekly CI stages (parameter sweeps + tuning) #361 switches it to-arch). This PR can be merged regardless of [AIROCMLIR-899] Enable weekly CI stages (parameter sweeps + tuning) #361, since the two don't conflict.Test Plan
On gfx942 (MI300); rocmlirTriton built with
-DROCMLIR_ENABLE_BENCHMARKS=hipblaslt:perfRunner.py --op=gemm --batch-allon a sample of tier1 gemm configs, to check the hipBLASLt baseline.perfRunner.py --op=fusiononresnet50-e2eandbert-torch-tosa-e2e, thencreateFusionPerformanceReports.py(with PR#361's-archchange applied locally, see Dependencies).LIT_FILTER=perf-scripts ninja check-rocmlir.Test Result
-transOcheck inck-gemm-benchmark-driver.cppisn't compiled yet, since Composable Kernel isn't installed locally and PR CI doesn't build the benchmark drivers It will first be built by the CK step of the nightly Benchmark stage added in AIROCMLIR-901.Submission Checklist