refactor(hipsparse): dedicated config header for build-time feature flags - #10263
Open
amontoison wants to merge 6 commits into
Open
refactor(hipsparse): dedicated config header for build-time feature flags#10263amontoison wants to merge 6 commits into
amontoison wants to merge 6 commits into
Conversation
…fig header Feature-flag macros (HIPSPARSE_WITH_SPMV_BSR, HIPSPARSE_WITH_CSC_TRSV, HIPSPARSE_WITH_CSC_TRSM) were baked into hipsparse-version.h and reached consumers only through transitive inclusion, mixing versioning with build configuration and making guard evaluation order-dependent. - Add generated hipsparse-config.h (from hipsparse-config.h.in) holding the build-time feature flags, each tagged with its "since"/"remove-after" release. - Trim hipsparse-version.h to version macros only. - Include hipsparse-config.h directly at every flag consumer (public headers, amd_detail/utility.h, and clients) so guard evaluation no longer depends on include order. - Add cmake/CheckFeatureFlags.cmake: a configure-time guardrail that fails the build if a public header is gated by a flag not registered in the config template.
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
amontoison
force-pushed
the
hipsparse_config_header
branch
from
August 1, 2026 00:14
397915b to
f821e8a
Compare
amontoison
changed the title
amontoison
commented
Aug 1, 2026
amontoison
commented
Aug 1, 2026
…orten guardrail comment - Tag HIPSPARSE_WITH_SPMV_BSR as introduced in 4.6 (BSR SpMV support shipped in hipSPARSE 4.6.0; confirmed by changelog and by the commit that first added the macro, which carried project(hipsparse VERSION 4.6.0)). - Order the feature flags newest-first (4.7 CSC flags above the 4.6 BSR flag). - Replace the long CheckFeatureFlags.cmake header comment with a concise one.
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (76.92%) 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 #10263 +/- ##
========================================
Coverage 69.62% 69.62%
========================================
Files 2741 2741
Lines 451544 451544
Branches 66531 66531
========================================
Hits 314370 314370
Misses 116966 116966
Partials 20208 20208
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
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
1 participant
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.
Motivation
This already worked, but only halfway.
The feature-flag macros (
HIPSPARSE_WITH_SPMV_BSR,HIPSPARSE_WITH_CSC_TRSV,HIPSPARSE_WITH_CSC_TRSM) were resolved by CMake into the installedhipsparse-version.hthrough#cmakedefine, so a consumer that pulled in ahipSPARSEheader did get the correct values.The catch is that it only held together by accident of include order. A
#ifdef HIPSPARSE_WITH_*saw the right value only ifhipsparse-version.hhad already been included at that point.If it hadn't, or if a stale
hipsparse-version.hfrom another install sat earlier on the include path, a guarded piece of public API could quietly disappear with no compile error.Versioning and build configuration were also mixed in the same header, and nothing ever verified that the whole thing stayed harmonized.
Every macro a header guards on is actually declared, and that a header using a flag actually pulls in the header that defines it.
What changed
hipsparse-version.hinto a new generatedhipsparse-config.h.hipsparse-version.hnow holds only the version macros.Each flag notes the release it was introduced in.
hipsparse-config.hdirectly (public headers, amd_detail/utility.h, and the clients) instead of relying onhipsparse-version.hbeing dragged in first.cmake/CheckFeatureFlags.cmake, run at configure time. It performs the two consistency checks that were missing before:a. every
HIPSPARSE_WITH_*macro used in a public header must be declared inhipsparse-config.h.in(otherwise the flag never reaches the installed header);b. every public header that uses such a macro must itself
#include "hipsparse-config.h"(so the value doesn't depend on include order).Notes
hipsparse-config.hships through the existing include-tree install, just likehipsparse-version.handhipsparse-export.h.