Repository navigation
feat(hipblaslt): select JIT solutions in heuristic queries - #13202
jaopaulolc wants to merge 4 commits into
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
b1900cf to
53196e2
Compare
53196e2 to
fda6a1c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## users/jolabega/jit-upstream-01-jit-interfaces #13202 +/- ##
=================================================================================
- Coverage 78.09% 78.08% -0.00%
=================================================================================
Files 3005 3005
Lines 522055 522117 +62
Branches 81485 81495 +10
=================================================================================
+ Hits 407656 407688 +32
- Misses 85475 85497 +22
- Partials 28924 28932 +8
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
jaopaulolc
left a comment
There was a problem hiding this comment.
Please address my comments.
2b879a2 to
d42ce30
Compare
jinchen62
left a comment
There was a problem hiding this comment.
Reviewed at 925ca44 against the base branch at f297932f, and built the host-only JIT configuration (HIPBLASLT_ENABLE_JIT=ON, HIPBLASLT_ENABLE_DEVICE=OFF) on gfx950.
Blocking
- The head doesn't compile with JIT enabled (inline on
hipblaslt-jit-backend.cpp:263). JIT run 40543334461 fails at the build step on gfx90a, gfx942 and gfx950. The description says this PR's checks don't include the JIT workflow, but it does run here, and it's red. - The JIT GPU tests fail independently of that. At
05a6166, before thewavesCountcommit, 9 of 10 failed (run 40538632774) with "plain K=512", "K=1024 did not keep both plain-pair solutions", and_WGM1chosen where_K512_WGM8is expected. #13111 fails the samejit-publishandjit-launchchecks at its head (run 40543243737), so this looks inherited from the base. The 12-of-12 result in the description is from2948ec5f6ae, before both. - Every JIT index lookup reloads the code object and keeps it (inline on
tensile_host.cpp:3490). I measured about 4 MiB of device memory per launch.
The other items are inline. Things I checked and ruled out: concurrent heuristic queries are safe, because the query state is thread_local; GemmRequest never copies alpha/beta from device memory, because there's no device-scalar pointer mode; and the override row can't overflow the results array, because requestedAlgoCount is decremented first. The provider-row filter and its cache key, the JIT-range collision check, the whole-struct heuristicResult_copy, and the null-solution guards in the name lookups look good.
I couldn't run the JIT tests end to end locally. Develop now needs a ROCm newer than 7.0 to build (hipDeviceAttributePciChipId), and ROCm 7.0's comgr aborts reading the committed kernels' metadata. The memory numbers come from calling SolutionAdapter::loadCodeObjectFile directly; the comments about generation cost come from reading the code.
a1d2150 to
4162910
Compare
4162910 to
3cfa9ff
Compare
1ee5961 to
d1db3d9
Compare
|
I reviewed this PR again. The following review threads are still unresolved, or comments have no author reply:
If you have already fixed these, please reply on each comment to say so and resolve the threads. |
d1db3d9 to
33a6160
Compare
|
I reviewed this PR again. The following review threads are still unresolved, or comments have no author reply:
If you have already fixed these, please reply on each comment to say so and resolve the threads. |
1 similar comment
|
I reviewed this PR again. The following review threads are still unresolved, or comments have no author reply:
If you have already fixed these, please reply on each comment to say so and resolve the threads. |
33a6160 to
ed1b5ff
Compare
|
Those three threads already have a reply and are resolved:
The review at #13202 (review) is the summary of those inline comments. The compile failure, the code-object reload, and the other inline items on that review are answered on their own threads. |
CongMa13
left a comment
There was a problem hiding this comment.
Two issues in the JIT selection and first-use generation paths are noted inline.
ed1b5ff to
dc855b7
Compare
CongMa13
left a comment
There was a problem hiding this comment.
No new issues found. I agree with the following existing comments:
- Mixed
wavesCountsemantics for JIT and prebuilt results — @jinchen62 — comment #4392314406 - Mode 1 provider-order coverage and test process isolation — @jinchen62 — comment #4392314507
- Confirm the JIT workflow's GPU runner cost with CI owners — @jinchen62 — comment #4392314518
- Propagate code-object load failures — @jinchen62 — comment #4392314531 (the fork-safety part was addressed; the load result is still discarded)
CongMa13
left a comment
There was a problem hiding this comment.
I found two P2 issues in the JIT CI integration: the provider-row implementation is missing from the workflow path filter, and the JIT lane is not documented in the component testing strategy.
I agree with the following existing comments:
- Mixed
wavesCountsemantics for JIT and prebuilt results — @jinchen62 — comment #4392314406 - Mode 1 provider-order coverage is skipped in CI — @jinchen62 — comment #4392314507
- Confirm the JIT workflow's GPU runner cost — @jinchen62 — comment #4392314518
- Propagate code-object load failures — @jinchen62 — comment #4392314531
- Correct the PR description's JIT-check statement — @jinchen62 — review #5671968669
| - "projects/hipblaslt/library/src/amd_detail/rocblaslt/src/handle.cpp" | ||
| - "projects/hipblaslt/tensilelite/include/Tensile/CachingLibrary.hpp" | ||
| - "projects/hipblaslt/tensilelite/include/Tensile/ExactLogicLibrary.hpp" | ||
| - "projects/hipblaslt/tensilelite/include/Tensile/ProviderRows.hpp" |
There was a problem hiding this comment.
This filter includes ProviderRows.hpp but omits projects/hipblaslt/tensilelite/src/ProviderRows.cpp, which implements the provider-row behavior used by heuristic selection. A later change to the implementation alone will skip the JIT workflow. Please add this path or a pattern that covers the implementation sources.
There was a problem hiding this comment.
Added in d1df9741553: projects/hipblaslt/tensilelite/src/ProviderRows.cpp is now in the paths: filter next to ProviderRows.hpp. I checked the filter with a script that applies GitHub path-filter matching to the tracked files: all entries match at least one file, including the new one (the top of the stack, #13209, adds the HipKittens entries and every one of its 24 entries matches).
| - "projects/hipblaslt/library/src/amd_detail/hipblaslt-ext.cpp" | ||
| - "projects/hipblaslt/clients/tests/jit/**" | ||
| - "projects/hipblaslt/scripts/jit/**" | ||
| - "projects/hipblaslt/library/src/amd_detail/rocblaslt/src/tensile_host.cpp" |
There was a problem hiding this comment.
Please add this JIT lane to projects/hipblaslt/TESTING.md, including its trigger paths, tested architectures, and whether it gates merges. That file is the component's current CI and gating reference, but it has no JIT lane entry.
There was a problem hiding this comment.
Done in d1df9741553. projects/hipblaslt/TESTING.md now lists the JIT lane in three places: the short version (not required), a new row in the Validation Gates table, and the Informational list under PR Test Classification. The row gives the workflow (hipBLASLt JIT CI, checks JIT (gfx90a), JIT (gfx942), JIT (gfx950)), what it runs (host-only build with HIPBLASLT_ENABLE_JIT=ON, the jit-cpu tests with every GPU hidden, then the jit-gpu tests, one GPU runner per architecture), the trigger (pull requests that change a path in the paths: filter, and manual dispatch) and the paths by area. On gating: it is not a required check. gh pr checks --required on the top PR lists only Component CI Summary, pre-commit and Math CI Summary, and the develop ruleset list in this file does not include a JIT job, so a red JIT job does not block a merge. The HipKittens kernel directory and its CMake file are added to that row in #13209 (9b3b5998ceb).
The attempt key omitted K and the operand types, so a second shape was never built, and a publish reloaded every code object already in use. Library-test children now start with posix_spawn, and setProblem drops prepared kernels on every build.
A matmul with no caller algorithm bound the prebuilt library before selection, so a JIT index from mode 1 or 2 could not be launched. Selection now follows the heuristic ranking and the launch resolves that index. Concurrent generation of one problem waits for the first attempt instead of building it twice.
JIRA ID
Motivation
Add JIT solution library and comgr builder can build a TensileLite kernel in the running process and store it on disk, but no query reads that store. This pull request connects those queries to the store. A kernel built for one query is reused by later queries and by later processes that can read the same directory. With the new setting left unset, the queries keep their current behavior.
Technical Details
HIPBLASLT_JITis read once per process inhipblaslt-jit-mode.cpp. Unset, empty or0leaves the existing query path.1places results from the JIT library after the prebuilt library's exact-size matches and before its other matches.2returns only JIT library results. Any other value warns once and leaves the setting off.docs/reference/env-variables.rstdocuments this variable andHIPBLASLT_JIT_LIBRARY_PATH. When that path is unset, the library root ishipblaslt-jit-<uid>under the system temporary directory.TMPDIRis not used.When the library has no matching kernel,
Jit::generatebuilds one in a private directory, checks that the solution accepts the problem, links the code object, and publishes it. The published index is in[0x40000000, 0x60000000), the range the library pull request reserves.hipblasLtMatmulsends an index in that range to the JIT library and every other index to the prebuilt library. If the prebuilt library already contains an index in the JIT range, the JIT library is disabled for that process.This pull request also declares the generation interfaces the library pull request leaves out:
Backend,SolutionStore,JitandmakeJitinhipblaslt-jit-component.hppandhipblaslt-jit-backend.hpp, thegetJitAlgo,getLibraryAlgosandmakeGemmRequestentry points inhipblaslt-jit.hpp, andDeviceTarget::make,makeLibraryStoreandRocblasltToTensileContractionProblemGemm.The backend in this pull request replays the committed test kernels from the library pull request. It publishes two copies. When K is a multiple of 512, the copy was built with workgroup mapping 8. The other copy, used for any K, was built with workgroup mapping 1. It does not call TensileLite's Python. Grouped GEMM is not filled from the JIT library. A build that was not given those kernels warns once: mode
1is then unchanged, and mode2returns no algorithms.flowchart TD Q["heuristic query"] --> M{"HIPBLASLT_JIT"} M -->|"unset or 0"| OLD["existing getBestSolutions path"] M -->|"1"| EQ["exact-size matches, then JIT library"] M -->|"2"| JL["JIT library only"] EQ --> JL JL -->|"no stored kernel"| GEN["build, check, publish"] GEN --> JL EQ --> OT["remaining prebuilt matches"]These edits are outside the JIT build flag and apply to every build:
RocBlasLtStatusToHIPStatusmapsrocblaslt_status_not_initializedandrocblaslt_status_type_mismatchto hipBLASLt statuses. It used to throwHIPBLAS_STATUS_INVALID_ENUMfor those two values.makeArgumentrecords the algorithm only aftersolvesucceeds, and it resetsinputs.Synchronizeron each call.getKernelNameFromAlgoIndexandgetSolutionNameFromAlgoIndexreturn an empty string for an unknown index instead of using a null solution.heuristicResult_copycopies the whole algorithm struct, not only its index bytes.CachingLibraryincludes the provider-row filter in its cache key. With no filter set, that key is the previousAllfilter.gemmCreateclears prepared kernels on every build.Gemm::runwith none returnsHIPBLAS_STATUS_NOT_INITIALIZEDinstead of launching nothing and returning success.setProblemfollowed byrunwithoutinitializefails the same way with JIT on and with JIT off.Add a HipKittens gfx950 JIT backend adds a second backend for gfx950. It is independent of replay, and a query asks each backend the build compiled.
Test Plan
On a host-only JIT build (
-DHIPBLASLT_ENABLE_JIT=ON -DHIPBLASLT_ENABLE_DEVICE=OFF),ctestonclients/tests/jitcovers the five tests from the library pull request plus:jit-libraryandjit-library-concurrencycheck publication, the index range, and concurrent readers.jit-end-to-endruns a replayed solution fromgetJitAlgothroughhipblasLtMatmulandhipblaslt_ext::Gemm, and compares the output with a host reference.jit-end-to-end-librarydoes the same for indices fromgetLibraryAlgos.jit-heuristic-off,jit-heuristic-fallbackandjit-heuristic-forcedcheck modes0,1and2. The fallback test checks the order (exact-size matches, then JIT, then the other prebuilt matches) only when the device library has an exact-size row. Otherwise it printsSKIPfor that check.clients/tests/src/provider_rows_gtest.cppcovers the provider-row filter. It is part ofhipblaslt-testand was not run for this description.Test Result
On gfx950, with
HIP_VISIBLE_DEVICES=4,ctestonclients/tests/jitpassed 12 of 12 at2948ec5f6ae. The later merged8e7db8c500only brings the source-bundle test's link fix from the parent branch. That merge was not re-run locally. gfx90a and gfx942 were not run on this machine. This pull request's checks do not include the JIT workflow. That workflow runs on the library pull request, where run 38434784675 passed on gfx90a, gfx942 and gfx950 foreda3a798cd6. The workflow's path list now includes the rocBLASLt files this pull request edits, so a change to those files retriggers it on a pull request that already runs the workflow.Submission Checklist
Risk level
Medium. Queries are unchanged when JIT is not built, or when
HIPBLASLT_JITis unset or0. Setting it to1or2changes which algorithms the query returns, and those kernels were not selected by a benchmark. Mode1places them ahead of every prebuilt match that is not an exact-size match.The directory is not pruned, so it grows with each new problem. Deleting the root while no process is using it clears it.
The status mapping for
rocblaslt_status_not_initializedandrocblaslt_status_type_mismatchchanges in every build, including builds with JIT off. The other items in the list above also apply to every build.Gemm::runwith no prepared kernels also returnsHIPBLAS_STATUS_NOT_INITIALIZEDon every build.setProblemdrops those kernels, so a laterrunwithoutinitializedoes not reuse them when JIT is off.Rollback is reverting this pull request, or leaving
HIPBLASLT_JITunset. The HipKittens pull request uses this branch as its base.