refactor(rocsparse): dedicated config header for build-time feature flags - #10262
Open
amontoison wants to merge 6 commits into
Open
refactor(rocsparse): dedicated config header for build-time feature flags#10262amontoison wants to merge 6 commits into
amontoison wants to merge 6 commits into
Conversation
…fig header Feature-flag macros (ROCSPARSE_WITH_ILDLT0, ROCSPARSE_WITH_U16_REMOVED, ROCSPARSE_WITH_HANDLE_CREATE, ROCSPARSE_WITH_ILU0_BOOST_SIGN) were baked into rocsparse-version.h and reached consumers only if that header happened to be pulled in (transitively) before a guard was evaluated. This mixed versioning with build configuration and was fragile: a stale rocsparse-version.h or a change in include order could silently drop guarded API. - Add generated rocsparse-config.h (from rocsparse-config.h.in) holding the build-time feature flags, each tagged with its "since"/"remove-after" release so the guards can be retired one release after the feature ships enabled. - Trim rocsparse-version.h to version macros only. - Include rocsparse-config.h directly at every flag consumer (public headers, internal headers, 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 that is not registered in the config template. Build-only flags (ASAN, MEMSTAT, ROCBLAS, CSC_TRSV, CSC_TRSM) are excluded explicitly.
❌ 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 |
- Replace the verbose "since/remove-after" per-flag tags with the concise "/* rocSPARSE X.Y */" style, matching hipSPARSE, ordered newest-first. Versions reflect the release in which each macro was introduced (verified via commit history): 5.0 for HANDLE_CREATE and U16_REMOVED, 4.7 for ILDLT0 and ILU0_BOOST_SIGN. - Shorten the CheckFeatureFlags.cmake header comment.
…ude the config header
Only MEMSTAT appears in a public header (rocsparse-auxiliary.h) while being build-only, so it is the only flag the guardrail can flag and thus the only one that needs excluding. ASAN, ROCBLAS, CSC_TRSV and CSC_TRSM are used only in src, never in public headers, so listing them was unnecessary (and would have masked a genuine mistake if one were later referenced from a public header).
…-generic.h The generated config header exists only in the build tree (.../include/rocsparse/rocsparse-config.h), never in the source tree, so the relative form '../rocsparse-config.h' failed to resolve when the source copy of internal/rocsparse-generic.h was compiled (it looked for library/include/rocsparse-config.h). Use the plain '#include "rocsparse-config.h"' form, which resolves through the build include path just like rocsparse-types.h and the other public headers.
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.
Motivation
This already worked, but only halfway.
The feature-flag macros (
ROCSPARSE_WITH_ILDLT0,ROCSPARSE_WITH_U16_REMOVED,ROCSPARSE_WITH_HANDLE_CREATE,ROCSPARSE_WITH_ILU0_BOOST_SIGN) were resolved by CMake into the installedrocsparse-version.hthrough#cmakedefine, so a consumer that pulled in arocSPARSEheader did get the correct values.The catch is that it only held together by accident of include order. A
#ifdef ROCSPARSE_WITH_*saw the right value only ifrocsparse-version.hhad already been included at that point.If it hadn't, or if a stale
rocsparse-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: that 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
rocsparse-version.hinto a new generatedrocsparse-config.h.rocsparse-version.hnow holds only the version macros.Each flag notes the release it was introduced in.
rocsparse-config.hdirectly (public headers, internal headers, and the clients) instead of relying onrocsparse-version.hbeing dragged in first.cmake/CheckFeatureFlags.cmake, run at configure time. It performs the two consistency checks that were missing before:a. every
ROCSPARSE_WITH_*macro used in a public header must be declared inrocsparse-config.h.in(otherwise the flag never reaches the installed header);b. every public header that uses such a macro must itself
#include "rocsparse-config.h"(so the value doesn't depend on include order).ROCSPARSE_WITH_MEMSTATis excluded from the check: it appears in a public header (rocsparse-auxiliary.h) but is build-only and never reaches consumers. Other build-only flags (ROCSPARSE_WITH_ASAN,ROCSPARSE_WITH_ROCBLAS,ROCSPARSE_WITH_CSC_TRSV,ROCSPARSE_WITH_CSC_TRSM) are used only insrc, never in public headers, so the guardrail never sees them.Notes
rocsparse-config.hships through the existing include-tree install, just likerocsparse-version.handrocsparse-export.h.