Repository navigation
Skip gfx950 attention perf configs that LLVM miscompiles in the sweeps - #559
Conversation
![rocmlir-pr-reviewer[bot]](https://avatars.githubusercontent.com/u/7440191?s=60&v=4){kind=small}
| # the kernel faults the GPU or returns wrong results (llvm/llvm-project#225054). | ||
| # TODO(ROCM-32030): Remove the entries once a fix for that issue reaches the | ||
| # pinned LLVM; see the parameter-sweep skip list in docs/bump_triton_version.md. | ||
| SKIPPED_PERF_CONFIGS = frozenset({ |
There was a problem hiding this comment.
mlir/utils/performance/ is listed under "Likely needs a back-port (shared with rocMLIR)" in docs/PR_REVIEW_CHECKLIST.md, so this PR needs one of the three back-port options in that section's Verdict subsection, and the description currently has none. Most likely (b) or (c) applies here — the skip list is keyed to the LLVM revision this repo pins via external/llvm-project, and parameterSweeps.py may not exist on the rocMLIR side at all. Please add a one-line note to the PR description, e.g. "No rocMLIR back-port: parameterSweeps.py is specific to the Triton-backed sweeps and the workaround is keyed to the LLVM revision pinned in external/llvm-project", or link the parallel ROCm/rocMLIR PR if the script is shared. (rocMLIR back-port check -- Verdict.)
| passes. All entries are gfx950 attention configs, found as these samples of | ||
| `attentionSweeps.py attention`: | ||
|
|
||
| | Seed | Sample | Data type | Failure without a fix | |
There was a problem hiding this comment.
This section tells the next LLVM bumper to "run the config with the entry removed on the listed chip and check that it passes", but the table has no column tying a row to one of the four SKIPPED_PERF_CONFIGS entries — the perf-config string carries no dtype, so Data type alone does not disambiguate the two bf16 / two i8 rows, and the problem shape needed to actually run the config is only recoverable from the seed and sample index. Either state explicitly that the rows are in the same order as the entries in SKIPPED_PERF_CONFIGS, or (better, since it keeps the provenance next to the thing it describes) put the seed/sample/dtype of each entry in a short comment above that entry in parameterSweeps.py and keep the table as the summary.
There was a problem hiding this comment.
Verdict: COMMENT · Findings: 3 (0 Critical, 2 Major, 1 Minor)
Scope
Two files. mlir/utils/performance/parameterSweeps.py gains a SKIPPED_PERF_CONFIGS frozenset of four (chip, SWEEP_KIND, perf_config) entries, an _is_skipped() helper, and a configs_to_run() generator inside sweep_parameters() that drops matching samples while still consuming param_iter; sweep_parameters() now returns a 5-tuple and run_config() prints the skip count. docs/bump_triton_version.md gains section 5.3.3 and a matching checklist entry so the list is re-evaluated on every LLVM bump.
Findings
mlir/utils/performance/parameterSweeps.py:490—mlir/utils/performance/is on the shared-with-rocMLIR path list, and the PR description has none of the (a)/(b)/(c) back-port options ("rocMLIR back-port check -- Verdict", Major).mlir/utils/performance/parameterSweeps.py:507— the skip relies on an exact match against the serialized perf-config string, so a perfConfig schema change silently turns every entry into a no-op (Major: missing assert on a non-trivial precondition).docs/bump_triton_version.md:354— the provenance table rows cannot be mapped back to individualSKIPPED_PERF_CONFIGSentries, which the section's own re-check instruction needs (Minor).
Notes
- Mechanics verified against the PR head:
SWEEP_KINDandperfconfigare defined on all four configuration classes (perfRunner.py:857/1170,1482/1706,1988/2024,2161/2242),perfconfigcarries theattn:/gemm:prefix the entries spell, andarchcan carry:sramecc+:xnack-suffixes so thesplit(':')[0]normalization is required.sweep_parameters()has exactly one caller (run_config(), updated here), so the arity change is contained. The re-indent at line 535 corrects the previous yapf continuation alignment. - Drawing the config and discarding it afterwards does keep the rest of each seed's sample identical — worth preserving if this code is refactored.
- Skipping on perf-config alone (not on perf-config plus problem shape) over-skips: a listed config drawn with an unaffected shape in a later week is dropped too. Acceptable while the list is four entries, but it means the sweeps permanently lose those tiles for any bug, not just ROCM-32030.
- These scripts have CI lint (flake8/yapf) but no functional tests, so no test-coverage finding is raised.
CI status
No failed or cancelled checks on 3180db5c7; the only pending entry is this auto-review job itself.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## users/bpetkovi/AIROCMLIR-899-enable-weekly-ci #559 +/- ##
=================================================================================
+ Coverage 84.59% 84.61% +0.02%
=================================================================================
Files 208 208
Lines 36263 36263
Branches 6549 6549
=================================================================================
+ Hits 30674 30681 +7
+ Misses 3349 3347 -2
+ Partials 2240 2235 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
37854fc
into
users/bpetkovi/AIROCMLIR-899-enable-weekly-ci
…361) * [AIROCMLIR-899] Enable weekly CI stages (parameter sweeps + tuning) Signed-off-by: bogdan-petkovic <bpetkovi@amd.com> * [AIROCMLIR-899] Align weekly sweep per-test timeout with rocMLIR (600s) Signed-off-by: bogdan-petkovic <bpetkovi@amd.com> * [AIROCMLIR-899] Use sh instead of undefined shStrict in weekly tuning stage * [AIROCMLIR-899] Restore pipefail for piped weekly tuning commands and use canonical --test-dir * [AIROCMLIR-899] Reuse buildProject in weekly stages and keep -x tracing in shStrict * [AIROCMLIR-899] Report config parse failures as ValueError so tuning skips foreign rows Signed-off-by: bogdan-petkovic <bpetkovi@amd.com> * [AIROCMLIR-899] Make the tuning verification timeout configurable and unbounded by default * [AIROCMLIR-899] Let --verify-timeout 0 disable the verification bound, use it in weekly Co-authored-by: Cursor <cursoragent@cursor.com> * [AIROCMLIR-899] Bound attention compile parallelism to survive the tuning OOM Co-authored-by: Cursor <cursoragent@cursor.com> * [AIROCMLIR-899] Bound GPU runs and collect weekly tuning failures instead of aborting Co-authored-by: Cursor <cursoragent@cursor.com> * [AIROCMLIR-899] Tune the weekly stage in a selectable tuning space The stage inherited tuningRunner's `full` default. A full run measured 39.6h on gfx90a for its five commands, with conv alone taking 25.7h, and did not finish at all on the other five chips. Expose the space as a `weeklyTuningSpace` choice defaulting to `quick`, which the same run measured 7 to 10 times faster per command, and keep `full` and `exhaustive` for a deliberate re-tune. The separate quick-DB commands now only run when the stage itself tuned a wider space, since otherwise they recompute what the main commands just did. Also drop the measurement-only knobs. `--gpu-run-timeout 300` treated a legitimately slow config as a hang and lost the whole problem, and `--num-cpus 8` bounded attention compiles only to survive that run, so `--abort-on-error` is restored in place of collecting failures. The verification bound stays disabled because verifying the largest tier1 gemm was measured at 104 minutes against the 600s default. * [AIROCMLIR-899] Pass the chip to rocmlir-driver as -arch in the fusion path The six rocmlir-driver invocations in perfRunner.py's fusion path still pass -targets, which came in with the initial import from rocMLIR and was orphaned when #80 restructured the driver's pipelines and dropped that option. rocmlir-driver rejects the flag, the stage produces no output, and because the call sends stderr to DEVNULL the failure surfaces much later as an IndexError on result[3] in get_fusion_test_info. Nothing exercised this until now. The fusion benchmarking calls sit in the commented-out nightly stage, and the fusion tuning calls only run from the weekly. In rocMLIR -targets takes a comma-separated list tied to the MHAL multi-target path while -arch takes a single target, and this code always passes one chip, so -arch is the faithful replacement. Verified by extracting the tuning key for every file under mlir/test/fusion/resnet50-e2e and mlir/test/xmir/bert-torch-tosa-e2e on gfx90a and gfx1201, 50 files in total, all of which now yield a test vector. * [AIROCMLIR-899] Keep the crash backtrace when a tuning pipeline dies format_error bounds reported output at ten lines, keeping the first and last five, which is right for a failed verification but wrong for a tuning driver that dies on a signal. The weekly hit an assertion in Triton's PaddedSharedEncodingAttr on gfx950 three runs in a row and the report elided the middle 39 frames, which are the ones naming the pass that constructs the attribute, leaving the failure undiagnosable. Raise the bound for this call site only. Verified that a 50 line stderr now survives whole instead of collapsing to 13 lines with an elision marker. * Skip gfx950 attention perf configs that LLVM miscompiles in the sweeps (#559) * Skip gfx950 attention perf configs that LLVM miscompiles in the sweeps * Check SKIPPED_PERF_CONFIGS keys against PERF_CONFIG_FIELD_NAMES at import * [AIROCMLIR-899] Recognize rocm-run RUN lines when extracting fusion tuning configs * [AIROCMLIR-899] Keep lit from reading the RUN line the find_run_command test writes lit takes every RUN: in a test file as a directive, so the f-string that writes a RUN line into the temporary .mlir became a second RUN line of the test itself and its script did not parse. Build the prefix from two strings. * [AIROCMLIR-899] Send Teams notifications for weekly CI --------- Signed-off-by: bogdan-petkovic <bpetkovi@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Muhamed-Husic <mhusic@amd.com>
3 participants
Motivation
This PR replaces the LLVM workaround in #552.
On gfx950, some attention configs in the weekly parameter sweeps hit an LLVM InlineSpiller bug (ROCM-32030, upstream llvm/llvm-project#225054), where the kernel faults or returns wrong results. Instead of patching LLVM, the sweeps skip the perf configs known to hit the bug until an upstream fix reaches our pinned LLVM. The fix proposed upstream, llvm/llvm-project#226689, does not cover our case (see llvm/llvm-project#226689 (comment)).
Technical Details
parameterSweeps.pygetsSKIPPED_PERF_CONFIGS, a list of (chip, sweep kind, perf_config) entries markedTODO(ROCM-32030).parameterSweeps.pyandattentionSweeps.pyskip a sampled config that matches an entry. The config is still drawn, so the rest of each week's sample is unchanged. Each skipped config prints aSKIPPED:line, and the count is printed after the summary.docs/bump_triton_version.mdgets a section and a checklist item for re-checking the entries on every LLVM bump.Test Plan
Test Result
SKIPPED_PERF_CONFIGS, in the same order.Submission Checklist