Repository navigation
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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #12888 +/- ##
===========================================
- Coverage 76.02% 73.59% -2.44%
===========================================
Files 2867 2862 -5
Lines 474745 473437 -1308
Branches 71078 70670 -408
===========================================
- Hits 360919 348383 -12536
- Misses 89313 100856 +11543
+ Partials 24513 24198 -315
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
talumbau
left a comment
talumbau
left a comment
There was a problem hiding this comment.
Thanks for splitting this out. I found two problems that I think would break installs (automatic RPM Requires, and the invoke build -n package globs) and three smaller packaging gaps. Details are in the inline comments.
talumbau
left a comment
talumbau
left a comment
There was a problem hiding this comment.
Re-reviewed the current head (db73c93fdcd) — my earlier comments were against 0cfd444eea5 and GitHub has marked them all outdated, so please ignore those. I built this branch for gfx942 and inspected the resulting RPMs rather than reading the diff alone.
Confirmed fixed since that revision: the origami-static install globs, tensilelite_standalone.cmake is gone, the ASAN Replaces/Breaks now resolve to hipblaslt-asan, and the global dependency carry-over works on the hipBLASLt side. The package split itself looks good:
tensilelite-host-1.5.0.70002 1.3 MB <- owns libtensilelite-host.so*
tensilelite-host-devel-1.5.0.70002 2.0 MB
origami-static-devel-1.5.0.70002 0.2 MB
hipblaslt-1.5.0.70002 1247.4 MB
hipblaslt Requires: rocm-core, roctracer >= 1.0.0, tensilelite-host
tensilelite-host Requires: rocm-core
Three things I'd raise.
1. The RPM dependency on tensilelite-host is unversioned, but the DEB one isn't. (projects/hipblaslt/CMakeLists.txt:948 vs :963)
# line 948 (RPM)
DEPENDS "${_tensilelite_runtime_package}")
# line 963 (DEB)
DEPENDS "${_tensilelite_runtime_package} >= ${_tensilelite_package_version}")Confirmed on the built package — rpm -qpR hipblaslt-1.5.0.70002-*.rpm gives a bare tensilelite-host with no version constraint. So on RPM any tensilelite-host satisfies any hipblaslt, which drops the version coupling the ticket asks for. The asymmetry with the DEB line looks unintentional.
2. Nothing Provides or Requires the soname.
$ rpm -qp --provides tensilelite-host-1.5.0.70002-*.rpm
tensilelite-host = 1.5.0.70002-.el9
tensilelite-host(x86-64) = 1.5.0.70002-.el9
No libtensilelite-host.so.1()(64bit). With AUTOREQPROV removed, the dependency is by package name only and nothing binds it to the ABI actually shipped — a SOVERSION bump wouldn't propagate to consumers. If the concern was the auto-Requires side-effects, CPACK_RPM_<COMPONENT>_PACKAGE_AUTOPROV ON gives the Provides without them. Happy to be told this is deliberate.
3. hipSPARSELt didn't get the same carry-over fix hipBLASLt got. (projects/hipsparselt/CMakeLists.txt:261-265)
rocm_package_add_dependencies(
COMPONENT runtime
DEPENDS tensilelite-host
)CPack uses CPACK_RPM_<COMPONENT>_PACKAGE_REQUIRES instead of the global CPACK_RPM_PACKAGE_REQUIRES once the component variable exists, so this drops rocm-core from the hipsparselt package — the same issue the new code at hipblaslt/CMakeLists.txt:934-945 handles correctly. Given hipBLASLt got the fix, this looks like an oversight rather than a decision. It also inherits finding 1 (unversioned).
Minor: HEADER_ONLY on rocm_create_package for a package that ships a real .so (tensilelite_packaging.cmake:117) reads oddly. Its effects appear gated on ROCM_USE_DEV_COMPONENT, which is set OFF eleven lines above, so I think it's inert rather than wrong — but what is it there for?
Caveat on my testing: I cloned with --shared, so git describe returned empty in the build container and the release fields came out blank (1.5.0.70002-.el9). That doesn't affect package names, Requires, Provides or file placement, but I have not verified release-string behaviour.
Not blocking on any of this — finding 1 is the one I'd want resolved before merge. Note the branch is currently conflicting with develop and ASAN is red, so there's a rebase coming anyway.
db73c93 to
ec576f9
Compare
libtensilelite-host.so was installed into hipblaslt's runtime component, so the only way for hipSPARSELt to obtain it at run time is to depend on the whole hipblaslt package. Measured on a 7.0.2 drop build that package is 1600.6 MB, of which 466.5 MB is Tensile kernel data hipSPARSELt never uses, to supply one 6.5 MB shared library. Move the LIBRARY/RUNTIME artefacts of tensilelite-host into a new "tensilelite" CPack component, named tensilelite-host via LIBRARY_NAME + PACKAGE_NAME. The static archive and the public headers stay in devel: they are build-time only and hipsparselt-devel already depends on hipblaslt-devel. The component is declared only when tensilelite-host is actually a shared library. The check reads the target TYPE property rather than BUILD_SHARED_LIBS, because that variable is assigned later in this file (~line 805) and is therefore not reliable at the packaging scope. Refs AIHPBLAS-4434, AIHPBLAS-3522. Co-Authored-By: Claude <noreply@anthropic.com>
libhipblaslt.so carries DT_NEEDED on libtensilelite-host.so.1, so the
hipblaslt runtime package must say so now that the library ships separately.
Three things are needed together; splitting them leaves the package in a
state that is worse than before.
1. Publish the soname. rocm-cmake's packaging defaults set
CPACK_RPM_PACKAGE_AUTOREQPROV to Off for every ROCm library, so rpmbuild's
ELF scanning never runs and nothing Provides or Requires a soname today.
AIHPBLAS-4434 assumed re-enabling it would be a repo-wide change; CPack in
fact supports per-component overrides (CPackRPM.cmake:1098-1106 falls back
from CPACK_RPM_<COMPONENT>_PACKAGE_<TAG> to the global tag), so it can be
scoped to the tensilelite component alone. tensilelite-host then Provides
libtensilelite-host.so.1()(64bit), derived from the binary rather than
hand-written, so the capability tracks SOVERSION automatically.
2. Pin the version. hipblaslt, hipsparselt and tensilelite-host are intended
to move together within one ROCm release, so the dependency is an exact
version rather than the >= floor used for hipblas-common.
CPACK_PACKAGE_VERSION is computed inside rocm_create_package
(ROCMCreatePackage.cmake:281,296) from PROJECT_VERSION_* plus
ROCM_LIBPATCH_VERSION, and rocm_package_add_*_dependencies must be called
before rocm_create_package, so the same value is recomputed here.
3. Preserve the global dependency list. CPack uses
CPACK_RPM_<COMPONENT>_PACKAGE_REQUIRES *instead of* the global
CPACK_RPM_PACKAGE_REQUIRES once the component variable exists, so adding
any runtime-scoped dependency silently drops the global ones. Without this
the hipblaslt package loses "roctracer >= 1.0.0" and "rocm-core"; measured
on a gfx942 build its runtime Requires became just /bin/sh plus the soname.
rocm-core is added explicitly because rocm_package_add_rocm_core_dependency
appends it from inside rocm_create_package, after this point.
Resulting hipblaslt runtime Requires:
libtensilelite-host.so.1()(64bit)
rocm-core
roctracer >= 1.0.0
tensilelite-host = 1.5.0.70002
The pin is on VERSION, not VERSION-RELEASE: two builds from different commits
share a VERSION and differ only in RELEASE, so this gives release lockstep,
not ABI safety. Only SOVERSION discipline (AIHPBLAS-3522 decision D7) gives
the latter.
Refs AIHPBLAS-4434.
Co-Authored-By: Claude <noreply@anthropic.com>
hipblaslt's runtime package pinned tensilelite-host to hipBLASLt's exact package version. That records "tensilelite version equals hipBLASLt version" in package metadata, which a standalone tensilelite package would have to undo. It also rebuilt CPACK_PACKAGE_VERSION by hand because the real value is computed later inside rocm_create_package, so a change to rocm-cmake's formula would break the pin silently. Remove the pin and the version recomputation. RPM keeps the soname dependency libtensilelite-host.so.1()(64bit). DEB gets no soname dependency, so it now depends on tensilelite-host with no version. Reword the install comment so the archive, headers and imported target living in hipblaslt-devel read as interim, not as the intended layout. Fix the TYPE-guard comment: BUILD_SHARED_LIBS at that point holds HIPBLASLT_BUILD_SHARED_LIBS, which is independent of TENSILELITE_BUILD_SHARED_LIBS. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ec576f9 to
c61b576
Compare
|
Thanks for rebuilding and checking the generated RPMs. I rebased the branch onto the current
I regenerated the packages and checked the actual RPM metadata. Both I left soname auto-provides disabled. That was deliberate in the earlier packaging review, and enabling |
talumbau
left a comment
talumbau
left a comment
There was a problem hiding this comment.
Re-reviewed and built the current head (c61b57624b6) for gfx942. Findings 1 and 3 from my previous review are resolved — RPM and DEB now both carry tensilelite-host >= ${_tensilelite_package_version}, and hipSPARSELt has the full globals + rocm-core carry-over. Thanks.
One new issue, which I'd like fixed before merge.
The TensileLite packages ship with an empty RPM Release / Debian revision.
hipblaslt VERSION=1.5.0.70002 RELEASE=c61b57624b6.el9
tensilelite-host VERSION=1.5.0.70002 RELEASE=.el9
tensilelite-host-devel VERSION=1.5.0.70002 RELEASE=.el9
origami-static-devel VERSION=1.5.0.70002 RELEASE=.el9
producing filenames like tensilelite-host-1.5.0.70002-.el9.x86_64.rpm. git describe resolved fine in that build — hipBLASLt picked up c61b57624b6 from the same tree — so this is specific to the nested packages.
Cause. rocm-cmake derives the release from PROJECT_VERSION_TWEAK:
# ROCMCreatePackage.cmake
elseif(PROJECT_VERSION_TWEAK)
string(REPLACE "-" "_" RPM_RELEASE ${PROJECT_VERSION_TWEAK})
endif()hipBLASLt sets that at CMakeLists.txt:43 via rocm_get_git_commit_tag(PROJECT_VERSION_TWEAK). But tensilelite/CMakeLists.txt:14 then calls
project(tensilelite VERSION "${TENSILELITE_PROJECT_VERSION}" LANGUAGES CXX)and TENSILELITE_PROJECT_VERSION is "1.5.0" — three components, so CMake sets PROJECT_VERSION_TWEAK to empty for that scope. Minimal repro:
OUTER tweak = 'abc123def'
SUB before project() tweak = 'abc123def'
SUB after project() tweak = ''
SUB inside block() tweak = ''
By the time tensilelite_packaging.cmake runs at line 259 — inside or outside the block() — the tweak is gone, so RPM_RELEASE is never set.
Suggested fix. Re-derive it after project(), the same way hipBLASLt does. In tensilelite/CMakeLists.txt, after the fetch_rocm_cmake include at line 21 so rocm_get_git_commit_tag is available:
# project() above resets PROJECT_VERSION_TWEAK from the VERSION argument, which
# has no fourth component. rocm-cmake builds the RPM Release and Debian revision
# from PROJECT_VERSION_TWEAK, so re-derive it here or the TensileLite packages
# ship with an empty release field.
rocm_get_git_commit_tag(PROJECT_VERSION_TWEAK)That covers both the nested and TENSILELITE_STANDALONE paths, since both call rocm_create_package from this scope. Setting CPACK_RPM_PACKAGE_RELEASE / CPACK_DEBIAN_PACKAGE_RELEASE in the environment would also work, but pushes the burden onto every caller.
Worth deciding explicitly whether TensileLite's release should track the same commit as hipBLASLt's, or be independent now that the project is independently versioned — the above gives the former.
I have not tested the fix, only the diagnosis. The mechanism is proven, but whether rocm_get_git_commit_tag is callable at that point depends on what fetch_rocm_cmake has pulled in by then — worth confirming rather than taking my word.
For completeness, I also missed this in my previous review: that build used a --shared clone, git describe failed inside the container, and every release came out blank, so I wrote it off as my own harness. It wasn't.
Still open from before, not blocking: nothing Provides or Requires the soname (libtensilelite-host.so.1()(64bit)), so the coupling is by package name only and a SOVERSION bump wouldn't propagate. CPACK_RPM_<COMPONENT>_PACKAGE_AUTOPROV ON would give the Provides without the auto-Requires side effects, if that was the concern.
|
Two things I'd like settled rather than defaulted, both about what the 1. soname coupling. Nothing currently Provides or Requires I'd like the safe option here. Provider side is one line in set(CPACK_RPM_TENSILELITE_PACKAGE_AUTOPROV "ON")
rocm_package_add_rpm_dependencies(COMPONENT runtime
DEPENDS "libtensilelite-host.so.1()(64bit)")The soname is derived from the binary by rpmbuild, so it tracks SOVERSION automatically and nobody has to remember to update it. 2. Worth stating plainly either way: neither of these catches ABI drift within soname 1. That remains AIHPBLAS-3522's D7 and is a SOVERSION-discipline commitment, not something packaging can enforce. |
Local package validation: minimum TensileLite version and package releaseThis reports local validation of the uncommitted candidate based on The candidate makes three packaging corrections:
It does not add Configuration and build matrixAll tests used the offline
The downstream test extracted the generated integrated RPMs into a clean prefix, verified that Representative commands: ROCM_LIBPATCH_VERSION=1001 cmake -S projects/hipblaslt -B <integrated-shared> \
-DGPU_TARGETS=gfx950 \
-DHIPBLASLT_ENABLE_DEVICE=OFF \
-DHIPBLASLT_ENABLE_CLIENT=OFF \
-DHIPBLASLT_ENABLE_ROCROLLER=OFF
cmake --build <integrated-shared> --target package --parallel 8
ROCM_LIBPATCH_VERSION=1001 cmake -S projects/hipblaslt/tensilelite -B <standalone-shared> \
-DGPU_TARGETS=gfx950 \
-DTENSILELITE_ENABLE_CLIENT=OFF \
-DTENSILELITE_BUILD_TESTING=OFF
cmake --build <standalone-shared> --target package --parallel 8Generated dependency and version metadataActual RPM query results: Generated Debian metadata uses the equivalent native syntax: Install manifestThe nested and standalone TensileLite RPM payload manifests are identical. The manifest below lists each distinct validated package layout once. hipblaslt runtime (9 entries)hipblaslt-devel (28 entries)tensilelite-host runtime (10 entries)tensilelite-host-devel (115 entries)origami-static-devel (26 entries)hipsparselt runtime (10 entries)hipsparselt-devel (14 entries)
|
Decision on
|
build(hipblaslt): package TensileLite independently (AIHPBLAS-4434) (#12888) JIRA ID : AIHPBLAS-4434 ## Motivation hipSPARSELt uses the TensileLite host library at run time but does not need hipBLASLt's large kernel-data package. The existing split placed TensileLite files in components owned by hipBLASLt, which also forced those packages to use the hipBLASLt version. This change makes TensileLite an independently versioned CMake and packaging project while preserving hipBLASLt and hipSPARSELt consumption. ## Technical Details - Define the TensileLite project version with `TENSILELITE_PROJECT_VERSION`, initially `1.5.0`, and install `tensilelite-config.cmake` with the `roc::tensilelite-host` export. - Generate independent shared, static-development, and AddressSanitizer packages through `rocm_create_package`. Nested package components use distinct names so they cannot enter hipBLASLt packages. - Keep the existing `make package` entry point by running the nested TensileLite CPack configuration after hipBLASLt packaging. - Make hipBLASLt depend on the matching TensileLite runtime and development package versions. Keep hipSPARSELt's CMake dependency on hipBLASLt while changing its runtime package dependency to `tensilelite-host`. - Preserve bundled Origami as a separate static development package and retain upgrade metadata for files formerly owned by hipBLASLt packages. ## Related - Work tracking: AIHPBLAS-4434 - Related PR: #12372 ## Device / Architecture Coverage This changes CMake exports and package construction without changing kernels, dispatch, or device behavior. Host builds used `gfx942` as the configured target; automated GitHub build and packaging checks are pending. ## Test Plan - Build and package hipBLASLt with shared and static TensileLite configurations. - Build and package TensileLite directly as a standalone project. - Verify AddressSanitizer packaging contains the TensileLite shared library and license. - Configure and link consumers of the shared and static `roc::tensilelite-host` exports and the `roc::hipblaslt` export. - Exercise the TheRock dependency-provider flow through Origami, hipBLASLt, and hipSPARSELt. - Upgrade from the pre-split Debian packages and check package-manager consistency. ## Test Result - Shared hipBLASLt build passed; one CPack invocation produced `hipblaslt`, `hipblaslt-dev`, `tensilelite-host`, `tensilelite-host-dev`, and `origami-static-dev`. - Static TensileLite build passed and produced `tensilelite-host-static-dev` and `origami-static-dev` without empty runtime or development packages. - Standalone and AddressSanitizer builds and Debian packaging passed. The AddressSanitizer package contains `libtensilelite-host.so` and depends on `rocm-core-asan`. - Shared and static package consumers configured and linked successfully. - TheRock Origami, hipBLASLt, and hipSPARSELt configure, build, install, and artifact classification passed. - Debian upgrade testing completed with `apt` status 0 and a clean `dpkg --audit` result. - The local full TensileLite Python suite was attempted: 7,845 passed and 452 skipped; 50 failed and 22 errored because the host lacks `/usr/bin/amdclang++` and gfx1250 support. The failures do not touch the changed CMake packaging paths. - Automated GitHub checks are pending. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/GOVERNANCE.md#pull-requests. ## Risk level Medium (3/5): this changes package ownership, dependency metadata, and installed CMake exports across TensileLite, hipBLASLt, Origami, and hipSPARSELt. It does not change runtime or device code.
TheRock Submodule Bump ActivityNewest first
|
## Why `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 is currently **no way to turn it on from outside the source tree**: - `install-host.sh` builds `cmake_common_options` from a closed set of flags — no `$@`, no `--cmake-args` passthrough - `tasks.py build()` has a fixed keyword list with no corresponding entry So any packaging build always takes the duplicating path. On a 7.0.2 drop build that means `libhipsparselt.so` carries 8,787 hidden TensileLite symbols and 5,774 `ContractionProblemGemm` strings, with no `DT_NEEDED` on `libtensilelite-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: | | current drop build | with `--therock` | |---|---|---| | `libhipsparselt.so` | 7.43 MB | 0.92 MB | | TensileLite symbols inside it | 8,787 | 127 | | `DT_NEEDED` on tensilelite | none | `libtensilelite-host.so.1` | It also activates the hipSPARSELt packaging work already merged in #12888. That dependency is gated on ```cmake if(HIPSPARSELT_ENABLE_HIP AND (HIPSPARSELT_ENABLE_THEROCK OR NOT BUILD_SHARED_LIBS)) ``` which is false in the current drop build, so no `tensilelite-host` dependency is emitted at all today — the shipped `hipsparselt` RPM requires only `rocm-core`. Without this PR, #12888's hipSPARSELt half never runs in the RPM path. ## What it changes Adds 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. 21 lines, purely plumbing. **Additive and opt-in. No defaults change** — the `add_subdirectory` path remains the default for standalone builds and is unaffected. ## Ordering Should land **after #12373**. Enabling `THEROCK=ON` without those fixes produces a hipSPARSELt whose clients fail to link (`dlange_`, `_gfortran_*`) and whose `hipsparselt-test` cannot find `libomp.so`, because `THEROCK=ON` also forces TheRock's `cblas` target — 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_declare` and does not use `install-host.sh` or `tasks.py`. Refs AIHPBLAS-3522, AIHPBLAS-4434. Co-authored-by: Claude <noreply@anthropic.com>
JIRA ID : AIHPBLAS-4434
Motivation
hipSPARSELt uses the TensileLite host library at run time but does not need hipBLASLt's large kernel-data package. The existing split placed TensileLite files in components owned by hipBLASLt, which also forced those packages to use the hipBLASLt version. This change makes TensileLite an independently versioned CMake and packaging project while preserving hipBLASLt and hipSPARSELt consumption.
Technical Details
TENSILELITE_PROJECT_VERSION, initially1.5.0, and installtensilelite-config.cmakewith theroc::tensilelite-hostexport.rocm_create_package. Nested package components use distinct names so they cannot enter hipBLASLt packages.make packageentry point by running the nested TensileLite CPack configuration after hipBLASLt packaging.tensilelite-host.Related
Device / Architecture Coverage
This changes CMake exports and package construction without changing kernels, dispatch, or device behavior. Host builds used
gfx942as the configured target; automated GitHub build and packaging checks are pending.Test Plan
roc::tensilelite-hostexports and theroc::hipblasltexport.Test Result
hipblaslt,hipblaslt-dev,tensilelite-host,tensilelite-host-dev, andorigami-static-dev.tensilelite-host-static-devandorigami-static-devwithout empty runtime or development packages.libtensilelite-host.soand depends onrocm-core-asan.aptstatus 0 and a cleandpkg --auditresult./usr/bin/amdclang++and gfx1250 support. The failures do not touch the changed CMake packaging paths.Submission Checklist
Risk level
Medium (3/5): this changes package ownership, dependency metadata, and installed CMake exports across TensileLite, hipBLASLt, Origami, and hipSPARSELt. It does not change runtime or device code.