Repository navigation
fix(kpack): route gfx1250v0 stepping directory to the gfx1250 bundle - #13780
Conversation
On A0-revision gfx1250 silicon, hipBLASLt/hipSPARSELt's loader reads exclusively from library/gfx1250v0/ (no fallback to library/gfx1250/, since base gfx1250 kernels give wrong results on A0). That directory ships with a complete Tensile index, but kpack's artifact splitter never assigned it to any architecture bundle: the per-arch directory regex didn't recognize the v0 stepping suffix, and the lazy/master index files end in .dat.zlib, which the filename-based check also doesn't match. The index was silently dropped during splitting, so every GEMM on A0 failed at load with HIPBLAS_STATUS_INVALID_VALUE. Teach the directory-arch regex to accept an optional v<N> stepping suffix, mapping gfx1250v0 to its base arch gfx1250 for bundling purposes (the two directories remain physically separate; only the bundle assignment changes). Apply the same mapping in the artifact verifier's architecture-separation check so it doesn't misclassify the stepping directory as foreign-arch contamination once the splitter starts routing it correctly. Fixes ROCM-30193.
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
marbre
left a comment
There was a problem hiding this comment.
Thanks @nunnikri, this looks sound, however, we hit this for the first time with gfx1250v0 and I am not sure a regex is the way to go. Could we eventually make the stepping mapping explicit for gfx1250v0 instead of accepting any v<N> suffix? hipBLASLt currently defines only gfx1250v0 as a revision subtree, and TensileLite models it as an explicit build alias. Its architecture parser also warns that names like gfx1250v1 can look valid to ISA parsing without being supported targets.
The current regex would silently route any matching gfx*v<N> directory to its base architecture. That works for gfx1250v0, but could misroute a future stepping directory whose kernels need a separate bundle. An explicit mapping would match the producer’s current contract and avoid assuming all revision suffixes share the base architecture’s packaging target.
Replace the generic "any arch + vN" directory pattern with an explicit
LIBRARY_ARCH_DIR_STEPPING_ALIASES lookup ({"gfx1250v0": "gfx1250"}),
mirroring TensileLite's own ARCH_BUILD_ALIASES table, which defines
this exact one entry and no others.
The previous (?:v0)? regex suffix still matched any base architecture
followed by v0 (e.g. a hypothetical gfx90av0 or gfx942v0), not just
the one verified case. Only gfx1250v0 is known to need this bundling
treatment; a future stepping directory for a different arch or
revision is not guaranteed to want the same treatment and should be
flagged, not silently assumed.
Shares the alias dict between database_handlers.py and
verify_artifacts.py (the latter now imports it) to avoid the two
staying in sync by hand. Adds negative test coverage proving
gfx1250v1 (wrong revision) and gfx90av0/gfx942v0 (wrong base arch)
are correctly rejected rather than merged, both at the handler level
and the architecture-separation verifier.
Thanks @marbre for the review. Updated accordingly |
geotseng-amd
left a comment
There was a problem hiding this comment.
LGTM. Verified end to end:
- Real-install split (local hipBLASLt + hipSPARSELt install, gpu_targets=[gfx1250, gfx1250-strict]):
- Base b7db38b: hipBLASLt gfx1250v0/ keeps only 218/437 files; all 219 .dat.zlib, including TensileLibrary_lazy_gfx1250.dat.zlib, land in blas_lib_generic. hipSPARSELt keeps 65/131. The verifier flags 283 files as "gfx1250v". This matches the nightly symptom in ROCM-30193.
- This PR (c9f67f7): all 437 + 131 gfx1250v0 files land in blas_lib_gfx1250; gfx1250/ and gfx1250-strict/ are unchanged; the architecture-separation check passes (1152 / 572 files).
- kpack suite: 503 -> 516 passed, failure set identical to base (pre-existing env-only failures).
- The new tests fail on the unfixed source (5 failures, exactly the bug cases), so they guard the regression.
- The explicit alias matches TensileLite's ARCH_BUILD_ALIASES, which has the single entry gfx1250v0 -> target gfx1250. Agree with marbre that this is better than a generic v regex.
Nit (non-blocking): the PR description's Technical Details still describes the optional v regex from the first commit. Please update it to the explicit alias table before squash-merge.
marbre
left a comment
There was a problem hiding this comment.
Thanks for updating the PR accordingly. My review agent pointed out the following:
The explicit alias is applied outside the library layouts that use it. The map is consulted by the shared _TensileHandler (/develop/rocm-systems/shared/kpack/python/rocm_kpack/database_handlers.py:178), which also handles rocBLAS. As a result, a file under rocblas/library/gfx1250v0/ can be assigned to the gfx1250 bundle, although the documented and tested alias is for hipBLASLt and hipSPARSELt. The verifier also normalizes any path component named gfx1250v0, without checking that it is under a relevant library/ directory (verify_artifacts.py (/develop/rocm-systems/shared/kpack/python/rocm_kpack/tools/verify_artifacts.py:339)).
I recommend scoping the alias to the relevant library handlers and directory context, so this exception can’t classify or validate unrelated paths.
As of today rocBLAS doesn't produce that directory thus this shouldn't be an issue today and it would not introduce a current regression. Thus this feedback is not blocking but restricting the alias to hipBLASLt and hipSPARSELt would be a tidy scope improvement. Wdyt?
Restrict the stepping-directory alias to the library subtrees it's
actually documented for. Previously LIBRARY_ARCH_DIR_STEPPING_ALIASES
was a flat {"gfx1250v0": "gfx1250"} dict consulted by the shared
_TensileHandler base class, which rocBLAS also uses -- so a
hypothetical rocblas/library/gfx1250v0/ would have been silently
merged into the gfx1250 bundle too, even though this alias is only
verified for hipBLASLt and hipSPARSELt. rocBLAS does not produce this
directory today, so this was not a current regression, but it was an
unverified assumption of exactly the kind this fix has been removing
throughout.
The dict is now keyed by library dir ({"hipblaslt/library": {...},
"hipsparselt/library": {...}}), and a new resolve_stepping_alias()
helper centralizes the matching logic so the verifier doesn't
duplicate it. The verifier's own stepping check previously
reconstructed the library-dir key via raw path-component index
arithmetic (index >= 2, directory_parts[index - 2]); that's now fully
encapsulated in the helper as a plain tuple-suffix comparison, with no
index arithmetic at the call site.
Adds test coverage proving the alias does not apply under
rocblas/library/, both at the handler level and the
architecture-separation verifier.
Yes i agree on that. This change should be very specific to the existing v0 folders. Updated the same in latest commit. Thanks again for your time on the review |
There was a problem hiding this comment.
No major blockers. Code and tests look good to me but I think your agent has been a bit to verbose and we can cut down the comment / docstring a bit.
Please update the PR description / squashed commit message before landing as this is still giving the state when you implemented the regex and it does not reflect the current state.
Co-authored-by: Marius Brehler <marius.brehler@amd.com>
Co-authored-by: Marius Brehler <marius.brehler@amd.com>
TheRock Submodule Bump ActivityNewest first
|
Motivation
On A0-revision gfx1250 silicon, hipBLASLt/hipSPARSELt's loader reads exclusively from
library/gfx1250v0/— by design, with no fallback tolibrary/gfx1250/, since basegfx1250 kernels give wrong results on A0. kpack's artifact splitter was silently
dropping the Tensile index from that directory during packaging, so every GEMM
dispatch on A0 failed at load with
HIPBLAS_STATUS_INVALID_VALUE. This is thestopgap fix to keep plain
gfx1250builds working on A0 during the ongoinggfx1250-stricttransition, per the explicit requirement that plaingfx1250builds must not regress in the meantime.
Technical Details
library/gfx1250v0/ships a complete Tensile index(
TensileLibrary_lazy_gfx1250.dat.zlib, etc.) alongside its.co/.hsacokernelfiles, but kpack's splitter never assigned the index to any architecture bundle:
the per-arch directory regex in
database_handlers.pydidn't recognize thev0stepping suffix, and the index files end in
.dat.zlib, which the filename-basedcheck also doesn't match. The
.co/.hsacofiles survived (filename-matched);the index didn't, so nothing could select/load them.
LIBRARY_ARCH_DIR_STEPPING_ALIASESis an explicit lookup table — keyed bylibrary (
"hipblaslt/library","hipsparselt/library") and, within each, by theexact stepping directory name (
"gfx1250v0") — mapping to the base arch(
"gfx1250") it belongs to. Deliberately not a general "any arch + vN"pattern: hipBLASLt currently defines only
gfx1250v0as a revision subtree(mirroring TensileLite's own
ARCH_BUILD_ALIASEStable, which has this exact oneentry and no others), and a future stepping directory — a different revision, a
different base architecture, or a different library — is not guaranteed to want
the same treatment, so it's intentionally left unmatched (and flagged, not
silently merged) until verified.
_TensileHandler(the base class thisalias lookup lives on) is shared by rocBLAS as well as hipBLASLt/hipSPARSELt.
Without the per-library scoping, a hypothetical
rocblas/library/gfx1250v0/would have been silently merged into the
gfx1250bundle too, even though thealias is only documented and verified for hipBLASLt/hipSPARSELt. rocBLAS doesn't
produce that directory today, so this wasn't a live regression, but it was the
same category of unverified assumption this fix otherwise removes.
verify_artifacts.py's architecture-separation checker shares the same lookupvia a
resolve_stepping_alias()helper indatabase_handlers.py, so the aliasdata and its matching rules live in exactly one place. The verifier's version
checks that the stepping directory is immediately inside the correct library's
own path components (a plain tuple-suffix comparison), not just present anywhere
in the path — this replaced an earlier, less readable version based on raw
path-component index arithmetic.
gfx1250/andgfx1250v0/) stay physically separate ondisk; only which published artifact they ship inside changes.
gfx1250-strictis unaffected — it remains its own, fully separate artifact.
from George Tseng (hipBLASLt); this PR carries that fix forward into
rocm-systems, narrowed from a generalv<N>regex to the explicit,library-scoped alias table above per review feedback.
Issue Tracking
JIRA ID: ROCM-30193
Test Plan
pytest shared/kpack/tests/, excludingtest_wheel_splitter.py, which fails to collect on an unrelated missingrocm_bootstrapdependency pre-existing in this environment).HipBLASLtHandler/HipSparseLtHandlertests cover directory-stepping detection,including the
.dat.zlibindex case that was previously dropped.unclassified (handler) or is flagged as contamination (verifier), for: a wrong
revision (
gfx1250v1), a wrong base architecture (gfx90av0,gfx942v0), andthe wrong library (
rocblas/library/gfx1250v0/).gfx1250v0/ships inside thegfx1250artifact end-to-end (splitter + verifier), and that
gfx1250-strictstaysseparate.
re-running the full suite against a clean checkout (same files reverted) and
confirming an identical failure set.
Test Result
614 passed, 32 failed, 5 skipped (full suite including pre-existing/unrelated
failures) — identical failure set with and without this change, confirming no
regressions. All new/updated tests for this fix pass.