-
Notifications
You must be signed in to change notification settings - Fork 27
Support P3371 by default #318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Calling it "std::mdspan" doesn't work for me.
P3371 got into C++26, so we can remove code for the case where the macro isn't defined.
P3371 got into C++26, so we don't need to support the non-P3371 case any more.
59c8f7a to
4798560
Compare
Signed-off-by: Christian Trott <crtrott@sandia.gov>
Signed-off-by: Christian Trott <crtrott@sandia.gov>
Signed-off-by: Christian Trott <crtrott@sandia.gov>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request implements support for P3371 ("Fix C++26 by making the rank-1, rank-2, rank-k, and rank-2k updates consistent with the BLAS") by making it the default behavior. The changes remove the LINALG_FIX_RANK_UPDATES CMake option and all associated conditional compilation, keeping only the P3371-compliant code.
Changes:
- Removed
LINALG_FIX_RANK_UPDATESCMake option and macro from build configuration - Kept P3371-compliant implementations in BLAS2/BLAS3 rank update functions (updating versions with separate input/output parameters)
- Removed conditional compilation branches for non-P3371 behavior
- Updated GoogleTest version from release-1.11.0 to v1.17.0
- Modified mdspan dependency configuration in CMake and CI workflows
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Removed LINALG_FIX_RANK_UPDATES option; changed target link from std::mdspan to mdspan |
| include/experimental/__p1673_bits/linalg_config.h.in | Removed LINALG_FIX_RANK_UPDATES macro definition |
| include/experimental/__p1673_bits/blas2_matrix_rank_1_update.hpp | Removed conditional compilation; kept P3371 updating semantics |
| include/experimental/__p1673_bits/blas2_matrix_rank_2_update.hpp | Removed conditional compilation; kept P3371 updating semantics |
| include/experimental/__p1673_bits/blas3_matrix_rank_k_update.hpp | Removed conditional compilation; kept P3371 updating semantics |
| include/experimental/__p1673_bits/blas3_matrix_rank_2k_update.hpp | Removed conditional compilation; kept P3371 updating semantics |
| tests/native/*.cpp (10 files) | Updated tests to use P3371-compliant API; removed old non-updating test code |
| tests/CMakeLists.txt | Updated GoogleTest version from release-1.11.0 to v1.17.0 |
| .github/workflows/cmake.yml | Changed mdspan_DIR path configuration |
| .github/workflows/macos.yml | Changed mdspan_DIR path configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| shell: bash | ||
| working-directory: stdblas-build | ||
| run: cmake $GITHUB_WORKSPACE/stdblas-src -Dmdspan_DIR=$GITHUB_WORKSPACE/mdspan-install/lib/cmake/mdspan -DLINALG_ENABLE_TESTS=On -DLINALG_ENABLE_EXAMPLES=On -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_INSTALL_PREFIX=$GITHUB_WORKSPACE/stdblas-install | ||
| run: cmake $GITHUB_WORKSPACE/stdblas-src -Dmdspan_DIR=$GITHUB_WORKSPACE/mdspan-install -DLINALG_ENABLE_TESTS=On -DLINALG_ENABLE_EXAMPLES=On -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_INSTALL_PREFIX=$GITHUB_WORKSPACE/stdblas-install |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mdspan_DIR path may be incorrect. According to the README.md (line 45), mdspan_DIR should point to ${MDSPAN_INSTALL_DIR}/lib/cmake/mdspan, not just ${MDSPAN_INSTALL_DIR}/. The CMake find_package command expects mdspan_DIR to point to the directory containing the mdspanConfig.cmake file, which is typically in the lib/cmake/mdspan subdirectory of the installation prefix. This change from the more specific path $GITHUB_WORKSPACE/mdspan-install/lib/cmake/mdspan to just $GITHUB_WORKSPACE/mdspan-install/ may cause CMake to fail to find the mdspan package.
| run: cmake $GITHUB_WORKSPACE/stdblas-src -Dmdspan_DIR=$GITHUB_WORKSPACE/mdspan-install -DLINALG_ENABLE_TESTS=On -DLINALG_ENABLE_EXAMPLES=On -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_INSTALL_PREFIX=$GITHUB_WORKSPACE/stdblas-install | |
| run: cmake $GITHUB_WORKSPACE/stdblas-src -Dmdspan_DIR=$GITHUB_WORKSPACE/mdspan-install/lib/cmake/mdspan -DLINALG_ENABLE_TESTS=On -DLINALG_ENABLE_EXAMPLES=On -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_INSTALL_PREFIX=$GITHUB_WORKSPACE/stdblas-install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm interesting. I guess lets try that in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Copilot Suppose that our changes are correct and the README is wrong. How would you suggest that we change the README file to reflect the correct behavior?
| shell: bash | ||
| working-directory: stdblas-build | ||
| run: cmake -S $GITHUB_WORKSPACE/stdblas-src -B $GITHUB_WORKSPACE/stdblas-build -Dmdspan_ROOT=$GITHUB_WORKSPACE/mdspan-install/ -DLINALG_ENABLE_TESTS=On -DLINALG_ENABLE_EXAMPLES=On -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_INSTALL_PREFIX=$GITHUB_WORKSPACE/stdblas-install | ||
| run: cmake -S $GITHUB_WORKSPACE/stdblas-src -B $GITHUB_WORKSPACE/stdblas-build -Dmdspan_DIR=$GITHUB_WORKSPACE/mdspan-install/ -DLINALG_ENABLE_TESTS=On -DLINALG_ENABLE_EXAMPLES=On -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_INSTALL_PREFIX=$GITHUB_WORKSPACE/stdblas-install |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mdspan_DIR path may be incorrect. According to the README.md (line 45), mdspan_DIR should point to ${MDSPAN_INSTALL_DIR}/lib/cmake/mdspan, not just ${MDSPAN_INSTALL_DIR}/. The original code used mdspan_ROOT which is a different CMake convention that accepts the installation prefix, but mdspan_DIR requires the specific path to the CMake config directory. This change may cause CMake to fail to find the mdspan package.
| run: cmake -S $GITHUB_WORKSPACE/stdblas-src -B $GITHUB_WORKSPACE/stdblas-build -Dmdspan_DIR=$GITHUB_WORKSPACE/mdspan-install/ -DLINALG_ENABLE_TESTS=On -DLINALG_ENABLE_EXAMPLES=On -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_INSTALL_PREFIX=$GITHUB_WORKSPACE/stdblas-install | |
| run: cmake -S $GITHUB_WORKSPACE/stdblas-src -B $GITHUB_WORKSPACE/stdblas-build -Dmdspan_DIR=$GITHUB_WORKSPACE/mdspan-install/lib/cmake/mdspan -DLINALG_ENABLE_TESTS=On -DLINALG_ENABLE_EXAMPLES=On -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DCMAKE_INSTALL_PREFIX=$GITHUB_WORKSPACE/stdblas-install |
Remove
LINALG_FIX_RANK_UPDATESCMake option and macro.Keep all code that was defined when the macro was defined,
and remove all code that was defined when the macro was not.
@youyu3 @dalg24 @crtrott