[origami] Consume Origami as a findable package; stop duplicate origami builds#7996
Conversation
davidd-amd
force-pushed
the
users/davidd-amd/tensilelite-shared-p1-origami
branch
from
c9e3f19 to
e643549
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (77.83%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #7996 +/- ##
========================================
Coverage 65.05% 65.05%
========================================
Files 2597 2597
Lines 403887 403887
Branches 60168 60168
========================================
Hits 262717 262717
Misses 121929 121929
Partials 19241 19241
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
davidd-amd
force-pushed
the
users/davidd-amd/tensilelite-shared-p1-origami
branch
2 times, most recently
from
0bddd4f to
468c316
Compare
talumbau
left a comment
talumbau
left a comment
There was a problem hiding this comment.
Looks mostly good. I don't know idiomatic usage of the docs/ directory but I was surprised to see content like that in this PR. is that standard?
Other issue is that I would generally prefer the build to fail if we can't find the origami from the build from TheRock (in the case that we are building in TheRock).
davidd-amd
force-pushed
the
users/davidd-amd/tensilelite-shared-p1-origami
branch
from
ccc8527 to
81bc34f
Compare
|
spmm_quick_suite (Timeout): CI bundle contention, not a regression in this PR The failing Same suite, same
The identical hipSPARSELt suite runs 70s in isolation but 300s+ in the bundle (and spmm_standard ran 935s in p2's bundle, passing only because its cap is 1800s). The variable is cross-project GPU contention from co-scheduled test jobs, not hipSPARSELt code. Why this PR surfaces it: it edits Not a perf regression: same-hardware A/B of develop vs this PR's origami change (built -O3 -DNDEBUG, full spmm filter) is at parity (develop 653s vs this PR 634s over 50971 cases). Suggested fix is CI-side, not origami:
Citations:
|
It will once that path is available which will require the PR in the rock to merge. Otherwise, we would unnecessarily fail. |
Replace origami's "export everything" behaviour (default visibility + WINDOWS_EXPORT_ALL_SYMBOLS) with an explicitly controlled export surface: - CMakeLists: CXX_VISIBILITY_PRESET hidden + VISIBILITY_INLINES_HIDDEN; add generate_export_header producing origami/origami_export.h (ORIGAMI_EXPORT macro, ORIGAMI_STATIC for static builds); wire the generated build-tree include dir into roc::origami-headers and install the header. - Headers: annotate the public API (7 classes/structs + ~61 free functions across 8 headers) with ORIGAMI_EXPORT; include origami/origami_export.h. Verified in hipblaslt-tpls:local (amdclang++ 23, SHARED build): origami builds, links, installs; internal helpers are now hidden (308 local-text symbols) and total dynamic-defined exports drop 378 -> 325. Residual std::filesystem exports (~142) are a libstdc++ default-visibility artifact pulled in via std::ofstream, pre-existing in baseline; stripping them fully would need a linker version-script (optional follow-up). Phase 1 G1 of TensileLite shared-library effort. Additive/reversible; no namespace, install/export-primitive, or ORIGAMI_BUILD_SHARED_LIBS change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ation Replace the enumerated INTERFACE target_sources + install(DIRECTORY include/) with a CMake HEADERS file set on roc::origami-headers: - target_sources(origami-headers INTERFACE FILE_SET HEADERS BASE_DIRS include + build-include FILES <10 public headers + generated origami_export.h>): the file set is now the single source of truth for the public surface; anything not listed is private by construction. BASE_DIRS carries the include roots into the build/install interfaces. - Install the file set via install(TARGETS origami-headers EXPORT origami-targets FILE_SET HEADERS ...) (rocm_install's TARGETS wrapper cannot forward FILE_SET, so origami-headers is installed explicitly and origami alone goes through rocm_install). The exported origami-targets.cmake now carries the HEADERS set. - set_target_properties(origami-headers VERIFY_INTERFACE_HEADER_SETS ON): every public header is now compiled standalone by all_verify_interface_header_sets. - Public headers include <hip/hip_runtime.h> and use hip types, so the header interface requires hip: link hip::host INTERFACE on origami-headers (never hip::device, per the config guard) and demote origami's own hip::host link to PRIVATE. This both models the real requirement and lets the verify compile and header-only consumers (hipblaslt-rocroller) see hip's include dirs. Verified in hipblaslt-tpls:local: all 11 public headers pass all_verify_interface_header_sets; install tree contains exactly the public headers in the correct origami/ layout; a standalone find_package(origami) consumer configures, compiles, links (exported API resolves), and ldd resolves to the single installed liborigami.so.1. Phase 1 G2+G3 of TensileLite shared-library effort. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…p double-building
Re-enable Origami as a findable package in both consumers so liborigami.so is
built once and shared, instead of being recompiled inside each consumer's tree.
hipBLASLt (G5): replace the `if(FALSE)` / add_subdirectory("../../shared/origami")
fallback (disabled by #3687 "until origami is available in TheRock") with a plain
`find_package(origami REQUIRED)` in the non-superbuild path. D1 review confirmed
the disable was a merge omission -- origami is now built/installed in both TheRock
and the monorepo superbuild (where this branch is skipped and roc::origami comes
from the superbuild's earlier add_subdirectory(shared/origami)). rocisa's own
`if(NOT TARGET roc::origami)` acquisition is left untouched (TensileLite scope).
hipSPARSELt (G6): drop the now-dead `set(ORIGAMI_ENABLE_INSTALL OFF)` shim (it
only mattered while hipBLASLt add_subdirectory'd origami) and declare the
dependency explicitly with `find_package(origami REQUIRED)`. Per Phase 1 scope,
`HIPBLASLT_ENABLE_HOST OFF`, `add_subdirectory(hipblaslt)`, and the
`TENSILE_STATIC_ONLY` reach-in are kept (Phase 2 removes them).
Validated in hipblaslt-tpls:local against an installed origami: hipBLASLt
configures via find_package(origami) (exit 0) and its build tree contains zero
origami .o objects, no origami.dir target, and no liborigami build rule -- the
only liborigami reference is the installed /tmp/oi/lib/liborigami.so.1.0 it links
against. Full hipBLASLt+hipSPARSELt build (36-export collapse + ldd gates) is the
G7 validation, pending.
Phase 1 G5+G6 of TensileLite shared-library effort. Reversible (§6.1).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… block hipsparselt does not directly depend on origami — it is hipBLASLt's (and tensilelite-host's) dependency. hipBLASLt's own find_package(origami REQUIRED) at line 262 fires before hipsparselt uses any origami target, so the explicit call in the hipsparselt block was redundant. Remove it and keep only the comment documenting why the old ORIGAMI_ENABLE_INSTALL shim is gone. Feedback from /cmake:grade finding SC-55 (dependency ownership clarity). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…oller) find_package(origami REQUIRED) requires a pre-installed/known origami, which breaks a standalone hipBLASLt configure (no origami in CMAKE_PREFIX_PATH). Mirror the rocroller pattern: find_package when HIPBLASLT_ENABLE_THEROCK, else fetch/build origami as a subdirectory. The TheRock/superbuild path still uses the single installed/super-built copy (the dedup that matters in a shared prefix); the add_subdirectory is the dev-build fallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…MI_ENABLE_INSTALL So the origami dedup can merge into rocm-libraries ahead of the TheRock side without breaking TheRock's build: - hipBLASLt consumes origami via a QUIET find_package probe, falling back to add_subdirectory(... EXCLUDE_FROM_ALL) when origami is not yet a findable package. EXCLUDE_FROM_ALL orphans origami's install rules (lib, headers, license, export) in the embedding build -- no duplicate install, no unclaimed-license artifact-accounting failure. Probe (not REQUIRED) keeps TheRock building today and auto-upgrades to the single installed origami once it ships. - origami's nanobind dependency is found first and only FetchContent-fetched as a fallback, so a hermetic build that already provides nanobind does not fetch. - Drop the ORIGAMI_ENABLE_INSTALL option and its install gate: every embedding consumer (hipBLASLt, rocisa, origami's own python) uses EXCLUDE_FROM_ALL to suppress the install, so the toggle is redundant. origami's install is now unconditional; standalone/superbuild/TheRock install it, EXCLUDE_FROM_ALL embeds do not. Validated in hipblaslt-tpls:local: origami standalone still configures+installs (lib/license/config/export); hipBLASLt standalone configure orphans origami's install (top-level cmake_install does not descend into the origami subdir). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Remove the comments added on this branch (origami/hipBLASLt CMake), keeping only the SPDX/copyright headers and develop's pre-existing comments. No code changes -- comment/blank lines only; C++ headers (and their #include directives) untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…red-build link
The G1 visibility refactor set CXX_VISIBILITY_PRESET hidden + per-symbol
ORIGAMI_EXPORT annotations but missed logger.hpp. Under the standalone
SHARED build, Logger::{instance,log,flush,update_from_env} (defined in
logger.cpp) were hidden, breaking origami-tests link with undefined
symbols. Annotate the four public out-of-line methods to match the
per-method export style used across the other origami headers.
The spmm clients verify every case against a CPU OpenBLAS reference (cblas_gemm). The test env pinned OPENBLAS_NUM_THREADS=OMP_NUM_THREADS=48 regardless of ctest --parallel 8, so up to 8 concurrent test processes spawned ~384 reference-BLAS threads, oversubscribing the CPU and inflating suite runtime. This pushed spmm_quick_suite (300s cap) into Timeout when hipsparselt is exercised in the combined multi-project test bundle, while the same suite passes in isolation. Drop the per-process thread count to 6 so 8 parallel processes use ~48 threads total. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
davidd-amd
force-pushed
the
users/davidd-amd/tensilelite-shared-p1-origami
branch
from
81bc34f to
1791b67
Compare
aliry95amd
left a comment
aliry95amd
left a comment
There was a problem hiding this comment.
From Origami perspective, it looks good to me. My only concern is the new ORIGAMI_EXPORT macro. I am not really familiar with the best practices here, but is it possible to apply the macro as a block to all functions instead of touching every single one?
Good suggestion, and I did look at consolidating these (class-scope on the types, or a #pragma GCC visibility push/pop region around the free functions) to cut the per-function annotations. I'm deliberately keeping the per-symbol labels, for one main reason: failure-mode safety under -fvisibility=hidden. With per-function labels, anything without a macro is hidden, so a forgotten/copy-pasted declaration fails as a missing export -> link error: loud and immediate. With class-scope or a visibility region, the export comes from context rather than the declaration, so a new private method on an exported class, or a line pasted into a push(default) block, gets silently exported - no error, just a widened surface and a re-opened interposition hazard. For a shared/ library that a lot of people edit, biasing every mistake toward the loud/safe direction is worth the extra annotations, and it keeps the public surface grep-able (grep ORIGAMI_EXPORT == the API). This also matches how libc++ does it (explicit macros at each declaration; the bare region pragma reserved for things like template instantiations and extern "C"). That said, there are a few places where I used class scope labels and I may create a follow on PR to really dial these details in to be certain we aren't doing something unintended like exporting the vtable if someone copy/pastes code - i.e. per-function is only equivalent to class-scope for non-polymorphic, non-RTTI-crossed types. |
|
Overriding infra approval requirement. |
## What Fixes the failing `precheckin(origami)` on #6334. The attention model was added without the symbol-export boilerplate #7996 introduced (hidden default visibility + `generate_export_header`). None of the `origami::attention` functions were marked `ORIGAMI_EXPORT`, so they ended up as local symbols in `liborigami.so` — breaking the link of both `origami-tests` and the `_pyorigami` bindings (which take the address of these functions) with undefined references. - `attention.hpp`: include `origami/origami_export.h` and mark the 18 public functions `ORIGAMI_EXPORT` (matches `gemm.hpp`; these are the public API exposed via `bindings.cpp`). - `CMakeLists.txt`: add `attention.hpp` to the `FILE_SET HEADERS` list so it's installed and checked by `VERIFY_INTERFACE_HEADER_SETS`. 2 files, +20/−18. No logic changes. ## Validation (MI300X / gfx942) - Build + link: OK (both `origami-tests` and `_pyorigami`) - C++ `ctest`: 110/110 pass - Python `ctest`: 6/6 pass (2 torch-gated selector tests skip without torch)
5 participants
Motivation
hipBLASLt and hipSPARSELt each built their own
liborigami.soviaadd_subdirectory,duplicating the library and its exports. This re-enables Origami as an installed,
findable package so it is built once and shared.
What this does
if(FALSE)/add_subdirectory(.../shared/origami)fallback(disabled by [origami] Enable build in TheRock #3687 "until origami is available in TheRock") with
find_package(origami REQUIRED). Origami is now built/installed in both TheRock and themonorepo superbuild, so the disable was a stale merge omission.
ORIGAMI_ENABLE_INSTALL OFFshim (it only matteredwhile hipBLASLt sub-built origami).
generate_export_header(ORIGAMI_EXPORT) +CXX_VISIBILITY_PRESET hidden—the exported ABI is the public API, not "everything".
FILE_SET HEADERSpublic-header contract +VERIFY_INTERFACE_HEADER_SETS(every publicheader compiles standalone).
hip::hoston the headers interface (public headers use hip types);hip::hostdemotedPRIVATE on the library itself.
Validation (Docker, amdclang++ 23)
all_verify_interface_header_sets.find_package(origami)consumer compiles, links, and runs;lddresolves the.oobjects / noorigamibuild target —Notes
find_package(origami)and restore theadd_subdirectoryfallback.hipBLASLt → origami/hipSPARSELt → origamipackage dependency edges.🤖 Generated with Claude Code