[AIROCMLIR-1163] Enable the vendored Triton lit test suite in nightly CI - #494
Conversation
| // Five sections expect the unconditional dot-operand hoist; downstream it is | ||
| // cost-gated, see triton-patches/patch-cost-aware-dot-rematerialization.patch. | ||
| // XFAIL: * | ||
|
|
There was a problem hiding this comment.
That patch is in the process of being upstreamed: https://github.com/triton-lang/triton/pull/11551/changes
Would it make more sense to just update that .patch file to properly update this LIT test?
There was a problem hiding this comment.
Thanks. I ported your https://github.com/triton-lang/triton/pull/11551/changes implementation into the vendored tree instead, with the test taken from that PR. Two bits could not come along because the pinned Triton does not have them yet (isRematBeneficial keeps its four-parameter signature, disableRematSplitting rejection omitted) and the tests pass now, with no XFAILs
| // RUN: triton-opt %s --allocate-shared-memory-nv='compute-capability=90 ptx-version=83' --convert-triton-gpu-to-llvm='compute-capability=90 ptx-version=83' --convert-nv-gpu-to-llvm | mlir-translate --mlir-to-llvmir | opt -O3 -S | llc -mtriple nvptx64-nvidia-cuda -mcpu=sm_90 -mattr=+ptx83 | FileCheck --check-prefixes CHECK,SM90 --dump-input-context=20 %s | ||
| // RUN: triton-opt %s --allocate-shared-memory-nv='compute-capability=80 ptx-version=83' --convert-triton-gpu-to-llvm='compute-capability=80 ptx-version=83' --convert-nv-gpu-to-llvm | mlir-translate --mlir-to-llvmir | opt -O3 -S | llc -mtriple nvptx64-nvidia-cuda -mcpu=sm_80 -mattr=+ptx83 | FileCheck --check-prefixes CHECK,SM80 --dump-input-context=20 %s | ||
| // RUN: triton-opt %s --allocate-shared-memory-nv='compute-capability=100 ptx-version=87' --convert-triton-gpu-to-llvm='compute-capability=100 ptx-version=87' --convert-nv-gpu-to-llvm | mlir-translate --mlir-to-llvmir | opt -O3 -S | llc -mtriple nvptx64-nvidia-cuda -mcpu=sm_100 -mattr=+ptx87 | FileCheck --check-prefixes CHECK,SM100 --dump-input-context=20 %s | ||
| // RUN: %if nvptx-registered-target %{ triton-opt %s --allocate-shared-memory-nv='compute-capability=90 ptx-version=83' --convert-triton-gpu-to-llvm='compute-capability=90 ptx-version=83' --convert-nv-gpu-to-llvm | mlir-translate --mlir-to-llvmir | opt -O3 -S | llc -mtriple nvptx64-nvidia-cuda -mcpu=sm_90 -mattr=+ptx83 | FileCheck --check-prefixes CHECK,SM90 --dump-input-context=20 %s %} |
There was a problem hiding this comment.
Is this something that we can/should upstream so that we don't have to maintain it ourselves?
There was a problem hiding this comment.
I think there is a good chance they take it. It changes no pass logic and no test expectations. It only states which RUN lines need the NVPTX backend, so a build without that backend skips those and keeps running the rest. For a build that has NVPTX nothing changes at all. I can open the upstream PR for it. The alternative was marking both files UNSUPPORTED, but that skips them everywhere, and tritongpu_to_ptx.mlir would also lose its three VEC RUN lines that need no backend
There was a problem hiding this comment.
@bogdan-petkovic can you try creating upstream patch ?
There was a problem hiding this comment.
Review feedback on #494: patch-cost-aware-dot-rematerialization is being upstreamed as triton-lang/triton#11551, and that version rejects the dot-operand hoist only when it would add expensive math, on the grounds that the payoff is pipelining the loads. Our vendored copy gated on total cost via isRematBeneficial, which also suppressed cheap chains and left five sections of test/TritonGPU/combine.mlir and all three functions of the mfma scaled-dot test failing. Port that implementation instead of rewriting the upstream expectations: getSliceOps, getValuesUsedOutsideSlice, RematerializationCost and getRematerializationCost, with the expensiveMathCost check at the hoist site. remove-layout-conversions-dot-operand-cost.mlir is byte-identical to the version in that PR. isRematBeneficial keeps its four-parameter signature and the disableRematSplitting rejection is omitted, because the pinned Triton predates that feature. Both upstream tests are byte-identical with upstream again and the XFAILs are gone. Suite: 289 tests, 286 passed, 3 unsupported. The record is regenerated from the pristine pinned sources, which also drops the malformed hunk header (21 declared new lines for a 20-line body) that made it impossible to reverse-apply.
Review feedback on #494. The first version derived the features from LLVM_TARGETS_TO_BUILD with a hand-written regex. configure_lit_site_cfg already exposes TARGETS_TO_BUILD, space separated, and llvm/test/lit.cfg.py already has this exact loop, so use that instead. Drops the regex and the re import, and makes the change trivially portable upstream.
There was a problem hiding this comment.
🔵 Needs a closer look
It combines nightly pipeline wiring with a substantial vendored compiler cost-model change, while the new nightly path remains unverified in submitted CI results.
Pull request overview
Enables Triton’s vendored lit suite in nightly CI and resolves regressions it exposed.
Changes:
- Adds CMake and Jenkins targets for building and running Triton lit tests.
- Gates NVPTX-dependent tests on registered LLVM targets.
- Aligns downstream tests and rematerialization behavior with upstream Triton.
File summaries
| File | Description |
|---|---|
cmake/triton.cmake |
Defines Triton lit targets and dependencies. |
mlir/utils/jenkins/Jenkinsfile |
Runs the suite in nightly CI. |
README.md |
Documents vendored suites. |
CONTRIBUTING.md |
Adds contributor test guidance. |
docs/bump_triton_version.md |
Adds suite to version-bump validation. |
triton-patches/triton-patch-content.txt |
Documents updated patch records. |
triton-patches/patch10497.patch |
Corrects the matmul pass option. |
triton-patches/patch11018.patch |
Updates GFX11 atomic expectations. |
triton-patches/patch-lit-registered-target-features.patch |
Records LLVM-target test gating. |
triton-patches/patch-cost-aware-dot-rematerialization.patch |
Records the revised cost model. |
external/triton/test/lit.cfg.py |
Exposes registered-target features. |
external/triton/test/lit.site.cfg.py.in |
Supplies configured LLVM targets. |
external/triton/test/Conversion/tritongpu_to_ptx.mlir |
Conditionally runs NVPTX pipelines. |
external/triton/test/Conversion/tritongpu_to_ptx_mmav3.mlir |
Requires the NVPTX backend. |
external/triton/test/TritonGPU/amd/accelerate-amd-matmul-fma-rdna.mlir |
Fixes the pass option. |
external/triton/test/TritonGPU/amd/amd-convert-buffer-ops-atomic-fminmax.mlir |
Corrects GFX11 expectations. |
external/triton/lib/Dialect/TritonGPU/Transforms/RemoveLayoutConversions.cpp |
Revises rematerialization cost handling. |
external/triton/test/TritonGPU/amd/remove-layout-conversions-dot-operand-cost.mlir |
Tests the revised cost policy. |
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![rocmlir-pr-reviewer[bot]](https://avatars.githubusercontent.com/u/7440191?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: APPROVE -- submitted as COMMENT (automated reviews are advisory) · Findings: 0 (0 Critical, 0 Major, 0 Minor)
Scope
Enables the vendored Triton lit suite (check-triton-lit-tests) in nightly CI. cmake/triton.cmake now adds external/triton/test (instead of only test/lib) from a wrapper function that supplies the find_package(MLIR)-derived variables Triton's lit.site.cfg.py.in expects, plus a check-triton-lit-tests-build-only aggregate target. The Jenkinsfile renames runsCheckMlir to runsVendoredSuites, builds the new target in the build stage, runs the suite in its own stage, and registers that stage in classifyBuildFailure. Eight vendored-tree edits fix the nine failures the suite exposed, each backed by a triton-patches/ record. Docs updated in README.md, CONTRIBUTING.md, and docs/bump_triton_version.md.
Findings
No blocking issues found.
Notes
Mechanical checks that came back clean:
- All three commits touching
external/triton/carry the[EXTERNAL]subject prefix (abe2425,6932e6e,ca63474); the non-vendored commits do not. The four touched.patchrecords correspond to the vendored-tree edits in the same commits, andtriton-patch-content.txtgains the newpatch-lit-registered-target-features.patchentry. TARGETS_TO_BUILDis populated by LLVM'sconfigure_lit_site_cfg, so the newconfig.targets_to_buildsubstitution and the<target>-registered-targetfeatures inlit.cfg.py:70-73are real. WithLLVM_TARGETS_TO_BUILDatX86;AMDGPU,nvptx-registered-targetis never set, so the gatedllcRUN lines skip whiletritongpu_to_ptx.mlirstill runs its threeVECchecks.LLVM_EXTERNAL_BIN_DIRmatches LLVM's runtime output dir andllvm-litis emitted byconfigure_fileat configure time, soget_llvm_lit_path'sEXISTSbranch is taken even on a fresh configure. Function-scopingLLVM_EXTERNAL_LITis safe because LLVM's earlieradd_lit_targetcalls already created the cache entry.LLVM_LIBRARY_DIRis set before the test dir is added, so Triton'sFILECHECK_PATHresolves.check-triton-lit-tests-build-onlymirrorsTRITON_TEST_DEPENDSplus the seven LLVM tools the RUN lines invoke unqualified.
Non-blocking observations, all outside the changed lines:
triton-patch-content.txt: the trailing prose underpatch-cost-aware-dot-rematerialization.patchstill describes the removed behaviour ("sumsgetConvertCostfor each load-leaf convert ... callsisRematBeneficial"), which contradicts the rewritten description above it. Worth a follow-up sweep of that entry.RematerializationCost::splitsSliceis written but never read. Intentional per the record (mirrors the upstream struct so the next bump can drop the record cleanly), so not flagged.- The PR description still mentions
LLVM_TOOLS_DIRas load-bearing; that variable was dropped inbd707f5. Description-only, no code impact. - The three records that do not reverse-apply in
sort -rorder (patch-warp-id-readfirstlane,patch-gfx1170,patch11118) are explicitly deferred to a separate ticket, which is the right call for this PR's scope.
Back-port check: every touched path is on the rocmlirTriton-only list, and the Jenkinsfile change is specific to the vendored Triton suite, so no rocMLIR back-port note is required.
CI status
Not assessed — /tmp/pr/checks.json was not examined in this run. Please confirm the PR and nightly runs independently; the nightly job is the only mode that exercises the new stage.
|
@bogdan-petkovic Can you fix the merge conflicts here ? |
bd707f5 to
66f4b7b
Compare
Fixed, I rebased it on develop as two clean commits and dropped the port since that went in with #521. The suite caught one more thing though, loop-pipeline-hip.mlir crashes since #548 because the swizzle check from Eric's port looks at the load layout before CoalesceAsyncCopy rewrites it. Fix is in #556, I cherry picked it here so the nightly runs with it and it drops out once #556 merges |
66f4b7b to
a08d3f9
Compare
Enabling check-triton-lit-tests exposes four failures in external/triton that come from our build configuration and our patches: - accelerate-amd-matmul-fma-rdna.mlir passed arch-generation-name, which is not an option of tritonamdgpu-accelerate-matmul in the pinned Triton, so the pass was never added. Upstream PR 10497 uses gfx-arch here. - amd-convert-buffer-ops-atomic-fminmax.mlir kept gfx1100/gfx1170 on NO-F32, but patch-buffer-atomics enables buffer atomic RMW there and GFX11 carries AtomicFMinFMaxF32GlobalInsts, so those rows are BUF-F32. - tritongpu_to_ptx.mlir and tritongpu_to_ptx_mmav3.mlir cannot resolve nvptx64-nvidia-cuda because we build X86;AMDGPU only. lit.cfg.py now adds the <target>-registered-target features LLVM's own lit.cfg.py derives from TARGETS_TO_BUILD. tritongpu_to_ptx.mlir gates only its llc RUN lines and keeps its three VEC checks, and tritongpu_to_ptx_mmav3.mlir, which needs the backend in every RUN line, requires it.
external/triton is our largest downstream surface and its lit suite was never run, so a Triton patch or bump could regress the vendored tree without any pipeline failing. cmake/triton.cmake now adds external/triton/test instead of only test/lib, which upstream gates behind TRITON_BUILD_UT (that would also pull in unittest/ and FetchContent(googletest)). The directory is added from a function so its lit variables stay local. LLVM_EXTERNAL_LIT points add_lit_target() at our llvm-lit, because LLVM exports its lit base dir only inside external/llvm-project and the target would otherwise run a bare /llvm-lit. LLVM is EXCLUDE_FROM_ALL, so FileCheck, count, not, split-file, llc, opt and mlir-translate are explicit dependencies, and check-triton-lit-tests-build-only lets CI build the suite in its build stage. Jenkins runs the suite in nightly on the mfma row, in its own stage next to check-mlir, with runsCheckMlir renamed to runsVendoredSuites. Nothing is built until the target is named, so PR builds pay nothing. README.md, CONTRIBUTING.md and docs/bump_triton_version.md describe the new suite. The test fixes from the previous commit are recorded in the new patch-lit-registered-target-features.patch and in patch10497.patch and patch11018.patch, with their entries in triton-patch-content.txt.
a08d3f9 to
7747efc
Compare
Motivation
external/tritonis our largest downstream surface, 28 patch records intriton-patches/against 18 inllvm-patches/, of which only four touchmlir/. Its lit suite was never run, so a Triton patch or a version bump could regress the vendored tree without any pipeline failing. 17 of our Triton patch records ship lit tests, and none of them were being executed. The suite has already caught one such regression, #548 brokeTritonGPU/loop-pipeline-hip.mlir, which #556 fixes.Technical Details
cmake/triton.cmakeaddsexternal/triton/testinstead of onlytest/lib, which gives us upstream'scheck-triton-lit-teststarget alongside theTritonTest*pass libraries thetriton-*binaries link. Upstream gatesadd_subdirectory(test)onTRITON_BUILD_UT, which would also pull inunittest/and itsFetchContent(googletest), so we keep UT off and add the directory ourselves.The directory is added from a function, so the variables it needs stay out of the rest of the build.
LLVM_EXTERNAL_LITis load-bearing, becauseadd_lit_target()builds the command as"${Python3_EXECUTABLE};${lit_base_dir}/llvm-lit"and LLVM exports that base dir only insideexternal/llvm-project, so without it the target runs a bare/llvm-lit.find_package(Python3)provides the interpreter for that command, andLLVM_LIT_TOOLS_DIRis only for the Windows tool lookup.configure_lit_site_cfgderives the LLVM directories itself. LLVM is addedEXCLUDE_FROM_ALL, soFileCheck,count,not,split-file,llc,optandmlir-translate, which Triton's RUN lines invoke unqualified, are explicit dependencies, since upstream'sTRITON_TEST_DEPENDSlists only thetriton-*binaries.check-triton-lit-tests-build-onlymirrorscheck-mlir-build-only, so CI builds the suite in its build stage.Jenkinsfile:
runsCheckMlirbecomesrunsVendoredSuitessince it now gates both vendored-tree suites under the same condition (params.nightly && codepath == "mfma").check-triton-lit-tests-build-onlyis built in the build stage, the suite runs in its own stage next tocheck-mlirbefore the GPU suite, and the stage name is registered inclassifyBuildFailure. Nothing is built until the target is named, so PR builds pay nothing.Docs updated:
README.md,CONTRIBUTING.md,docs/bump_triton_version.md(Step 2.2, Step 12 and the checklist).Failures the suite exposes on develop
Four come from our build configuration and patches and are fixed here, each with a patch record:
accelerate-amd-matmul-fma-rdna.mlirpassedarch-generation-name, which is not an option oftritonamdgpu-accelerate-matmulin the pinned Triton, so the pass was never added and the test failed in all four sections. Upstream PR 10497 hasgfx-archhere, so this restores the RUN line to the upstream form (patch10497.patchupdated).amd-convert-buffer-ops-atomic-fminmax.mlirkept gfx1100/gfx1170 onNO-F32. Upstream reaches those families with no buffer atomic RMW at all, butpatch-buffer-atomicsenables it and GFX11 carriesAtomicFMinFMaxF32GlobalInsts, so the rows move toBUF-F32(patch11018.patchupdated, cross-referenced frompatch-buffer-atomics).nvptx64-nvidia-cudabecause we buildX86;AMDGPUonly.lit.cfg.pynow adds the<target>-registered-targetfeatures LLVM's own suites use, from theTARGETS_TO_BUILDthatconfigure_lit_site_cfgexposes.tritongpu_to_ptx.mlirgates only itsllcRUN lines and keeps its threeVECchecks, andtritongpu_to_ptx_mmav3.mlirrequires the backend (newpatch-lit-registered-target-features.patch). Generic and upstreamable, not a permanent fork, and filed upstream as [lit] Add registered-target features and use them for NVPTX llc tests triton-lang/triton#12072.The fifth,
TritonGPU/loop-pipeline-hip.mlir, is a regression from #548 in the direct-to-LDS swizzle clamp. #556 fixes it, but that fix waits for upstream review first, so the test is marked XFAIL here (newpatch-xfail-loop-pipeline-hip.patch). With the #556 changes the test passes unexpectedly, so #556 drops the XFAIL when it lands. The failures the suite showed earlier on this branch are gone from develop. The consan pair andpipeline-split-cluster-unscheduled-op.mlircame from theNDEBUGmismatch #491 fixed, andcombine.mlirand the mfma scaled-dot test were fixed by porting the upstream version of the cost-aware dot remat patch in #521.Not fixed here, for a separate ticket:
patch-warp-id-readfirstlane.patch,patch-gfx1170.patchandpatch11118.patchdo not reverse-apply in the documentedsort -rorder, because they share test files and their required order is the opposite.Test Plan
Locally, on develop with this branch:
ninja check-triton-lit-testsninja check-mlir, to confirm the new suite does not cross-talk with the existing lit configscheck-rocmlirTest Result
check-triton-lit-tests: 291 tests, 287 passed, 1 expected failure (loop-pipeline-hip.mlir), 3 unsupported. With the [EXTERNAL] Check the direct-to-LDS swizzle against the chunk a warp writes #556 changes applied,loop-pipeline-hip.mlirpasses unexpectedly, which is what tells [EXTERNAL] Check the direct-to-LDS swizzle against the chunk a warp writes #556 to drop the XFAIL.check-mlir: 4214 tests, 3617 passed, 596 unsupported, 1 expected failure.check-rocmlir: everything that runs passes except theperf-scriptstests, which need pandas on my machine.Submission Checklist