Skip to content

Conversation

@mhoemmen
Copy link
Contributor

Remove LINALG_FIX_RANK_UPDATES CMake 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

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.
Signed-off-by: Christian Trott <crtrott@sandia.gov>
Signed-off-by: Christian Trott <crtrott@sandia.gov>
Signed-off-by: Christian Trott <crtrott@sandia.gov>
Copy link

Copilot AI left a 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_UPDATES CMake 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
Copy link

Copilot AI Jan 23, 2026

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
Copy link
Member

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.

Copy link
Contributor Author

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
Copy link

Copilot AI Jan 23, 2026

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants