Repository navigation
Expose HIPSPARSELT_ENABLE_THEROCK through the build drivers - #12374
Merged
Merged
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. |
talumbau
marked this pull request as ready for review
September 23, 2026 23:18
talumbau
force-pushed
the
tja/tlh-r4-therock-flag
branch
from
September 23, 2026 23:22
de70062 to
62e1e73
Compare
talumbau
force-pushed
the
tja/tlh-r4-therock-flag
branch
from
October 16, 2026 18:22
62e1e73 to
8f45c98
Compare
HIPSPARSELT_ENABLE_THEROCK selects the path where hipSPARSELt consumes a staged hipBLASLt package via find_package and links the shared libtensilelite-host.so, instead of pulling hipBLASLt in with add_subdirectory and recompiling TensileLite into libhipsparselt.so. The option defaults OFF and there was no way to turn it on from outside the source tree: install-host.sh builds cmake_common_options from a closed set of flags with no passthrough, and tasks.py build() has a fixed keyword list. Packaging builds therefore always took the duplicating path -- a 7.0.2 drop build shows 8,787 hidden TensileLite symbols and 5,774 ContractionProblemGemm strings inside libhipsparselt.so, and no DT_NEEDED on libtensilelite-host.so.1. Add a --therock flag to install-host.sh and a matching therock= keyword to tasks.py build(), so a CI driver can select the shared path. Additive and opt-in; no defaults change. The add_subdirectory path remains the default for standalone builds and is unaffected. Refs AIHPBLAS-3522, AIHPBLAS-4434. Co-Authored-By: Claude <noreply@anthropic.com>
talumbau
force-pushed
the
tja/tlh-r4-therock-flag
branch
from
October 16, 2026 21:51
8f45c98 to
40dc3f9
Compare
talumbau
added a commit
that referenced
this pull request
Oct 19, 2026
…12373) ## Why hipSPARSELt has accumulated several undeclared dependencies on hipBLASLt's **configure-time side effects**. When hipBLASLt is pulled in via `add_subdirectory()`, its `CMakeLists.txt` runs in the same configure and hipSPARSELt silently inherits settings from it. Consuming hipBLASLt as a package instead removes those, and nothing replaces them. **Nothing shipping today is broken.** Both current build paths work: | Build | `THEROCK` | BLAS it resolves to | clients | |---|---|---|---| | ROCm drop RPMs | OFF | BLIS + netlib LAPACK, statically linked | fine | | TheRock nightly | ON | `librocm-openblas.so.0` | fine | | staged-package build outside TheRock | ON | netlib CBLAS — no LAPACK | **fails** | That third configuration is the one #12374 is about to enable for the drop pipeline, in order to get the TensileLite deduplication into the RPMs. This PR makes it work before it gets switched on. These are three independent fixes, each defensible on its own: - a Fortran/LAPACK setup gated on the wrong condition, - a BLAS selector coupled to an unrelated flag, - a project relying on another project's cache variable. ## What **1. Scope the Fortran/LAPACK setup to configurations that need it.** It was gated on `NOT HIPSPARSELT_ENABLE_THEROCK` while `add_subdirectory(clients)` ran unconditionally. That works in TheRock because its `cblas` target is `rocm-openblas`, which bundles BLAS, CBLAS, LAPACK and the Fortran runtime in one library — verified against a TheRock nightly, where `hipsparselt-test` and `hipsparselt-bench` both carry `DT_NEEDED` on `librocm-openblas.so.0` and resolve `dlange_` from it. Outside TheRock the same branch resolves to the netlib CBLAS built by `fetch_lapack`, which has no LAPACK and does not pull the Fortran runtime: ``` ld.lld: error: undefined symbol: _gfortran_st_write ld.lld: error: undefined symbol: dlange_ ``` Now gated on the same selector the BLAS choice uses: `NOT THEROCK OR BLIS`. >⚠️ Deliberately **not** widened to "whenever clients are built". `HIPSPARSELT_ENABLE_CLIENT` defaults `ON` and TheRock does not override it, so an unconditional `enable_language(Fortran)` + `find_package(LAPACK)` would run in TheRock builds — where `HIPSPARSELT_ENABLE_FETCH=OFF` sends a failed `find_package(LAPACK)` into the `FATAL_ERROR` branch of `fetch_lapack.cmake`. An earlier revision of this commit did exactly that and would have broken TheRock. **2. Decouple the client BLAS choice from `HIPSPARSELT_ENABLE_THEROCK`.** That option selects two unrelated things: how hipBLASLt is consumed, *and* which CPU reference BLAS the clients link. `hipsparselt_link_blas_libraries()` linked a bare `cblas` target whenever THEROCK was on, which is only correct where `cblas` is OpenBLAS. The explicit BLAS selection is now checked first. **3. Declare hipSPARSELt's install RPATH.** hipSPARSELt never set `CMAKE_INSTALL_RPATH`; it inherited hipBLASLt's, set as a **cache** variable at `projects/hipblaslt/CMakeLists.txt:114`. On the staged-package path that never runs: ``` add_subdirectory : $ORIGIN/../lib:$ORIGIN/../llvm/lib:$ORIGIN/../lib:... staged package : $ORIGIN/../lib:$ORIGIN/../lib/hipsparselt/lib:... ``` `libomp.so` lives in `../llvm/lib`, so `hipsparselt-test` failed at startup with `error while loading shared libraries: libomp.so`. ## TheRock impact Commits 1 and 2: **none**, verified. TheRock's flags (`math-libs/BLAS/CMakeLists.txt:366`) leave `HIPSPARSELT_ENABLE_BLIS` at its `OFF` default, so it still takes the `cblas` branch and still skips the Fortran setup. Configure-only runs against a staged hipBLASLt package: | Configuration | Fortran block | `fetch_lapack` FATAL | BLAS chosen | configure | |---|---|---|---|---| | `THEROCK=ON, BLIS unset, FETCH=OFF` *(TheRock's actual flags)* | no | no | TheRock OpenBLAS | OK | | `THEROCK=ON, BLIS=true, FETCH=ON` | yes | no | BLIS + LAPACK | OK | | `THEROCK=OFF, BLIS=true` *(current drop default)* | yes | no | BLIS + LAPACK | OK | Row 1 matches TheRock's behaviour before this series exactly.⚠️ **Commit 3 is a real behaviour change for TheRock and wants a sign-off.** TheRock passes no `-DCMAKE_INSTALL_RPATH`, so where its hipSPARSELt binaries previously had no install RPATH they will now get `$ORIGIN/../lib:$ORIGIN/../llvm/lib`. Low risk — relative RPATH entries that do not resolve are skipped by the loader, and TheRock can override by passing the variable, since the `set()` uses `CACHE` without `FORCE`. If you would rather not take it, the fallback is to gate the `set()` on `NOT HIPSPARSELT_ENABLE_THEROCK`. ## Testing gfx942, ROCm 7.0.2, RHEL 9.5. - `hipsparselt-test --gtest_filter=*smoke*:*quick*:*pre_checkin*-*known_bug*`: **77,993 passed** - `hipblaslt-test --gtest_filter=*smoke*`: **3,118 passed** - **`THEROCK=OFF` path re-verified** after these fixes, against the original drop RPMs: `DT_NEEDED` on tensilelite 0 → 0, `Requires` `rocm-core` → `rocm-core`, RUNPATH still contains `../llvm/lib`. Unchanged. Refs AIHPBLAS-4434, AIHPBLAS-3522. --------- Co-authored-by: Claude <noreply@anthropic.com>
davidd-amd
approved these changes
Oct 19, 2026
hcman2
approved these changes
Oct 28, 2026
hcman2
left a comment
Contributor
There was a problem hiding this comment.
OK for me. better to have Vin's approval
Collaborator
Author
|
@vin-huang I see @hcman2 's comment now. Please advise on follow ups if needed. Trying to get this in before next scheduled drop on 10/29. |
Contributor
TheRock Submodule Bump ActivityNewest first
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
HIPSPARSELT_ENABLE_THEROCKselects the path where hipSPARSELt consumes a staged hipBLASLt package viafind_packageand links the sharedlibtensilelite-host.so, instead of pulling hipBLASLt in withadd_subdirectoryand recompiling TensileLite intolibhipsparselt.so.The option defaults
OFFand there is currently no way to turn it on from outside the source tree:install-host.shbuildscmake_common_optionsfrom a closed set of flags — no$@, no--cmake-argspassthroughtasks.py build()has a fixed keyword list with no corresponding entrySo any packaging build always takes the duplicating path. On a 7.0.2 drop build that means
libhipsparselt.socarries 8,787 hidden TensileLite symbols and 5,774ContractionProblemGemmstrings, with noDT_NEEDEDonlibtensilelite-host.so.1.What this unblocks
This is the switch that lets the hipBLASLt/hipSPARSELt drop pipeline pick up the TensileLite deduplication from #12888. Measured on gfx942:
--therocklibhipsparselt.soDT_NEEDEDon tensilelitelibtensilelite-host.so.1It also activates the hipSPARSELt packaging work already merged in #12888. That dependency is gated on
which is false in the current drop build, so no
tensilelite-hostdependency is emitted at all today — the shippedhipsparseltRPM requires onlyrocm-core. Without this PR, #12888's hipSPARSELt half never runs in the RPM path.What it changes
Adds a
--therockflag toinstall-host.shand a matchingtherock=keyword totasks.py build(), so a CI driver can select the shared path. 21 lines, purely plumbing.Additive and opt-in. No defaults change — the
add_subdirectorypath remains the default for standalone builds and is unaffected.Ordering
Should land after #12373. Enabling
THEROCK=ONwithout those fixes produces a hipSPARSELt whose clients fail to link (dlange_,_gfortran_*) and whosehipsparselt-testcannot findlibomp.so, becauseTHEROCK=ONalso forces TheRock'scblastarget — which outside TheRock is netlib CBLAS with no LAPACK. #12373 separates those two concerns.The companion rocJenkins change that actually passes the flag is held until this merges.
TheRock impact: none
TheRock configures CMake directly via
therock_cmake_subproject_declareand does not useinstall-host.shortasks.py.Refs AIHPBLAS-3522, AIHPBLAS-4434.