Fix stride(0) ordering, bound incx, reject incx == 0, deduce the CBLAS index type, detect padded layouts, and build the CBLAS path in CI

[georgemalerbo]

Applies what @mhoemmen suggested in #331 x.stride(0) is now only called after x.is_strided() is confirmed, incx is bounded by the index type's max, and incx == 0 returns false so generic fallback handles it.

Also implemented the deduction of the BLAS integer index type discussed in #328: a variadic function template declaration, used only inside decltype, extracts the type of cblas_dscal's first parameter

[mhoemmen]

Thanks for the contribution! : - )

The code inside #ifdef __cpp_lib_submdspan ... #endif is not well formed. This suggests that we don't have test coverage for that case. Would you consider adding tests for layout_left_padded and layout_right_padded? The tests should not be gated on __cpp_lib_submdspan, for the reasons explained in the comment on that part of the code. Thanks!

[mhoemmen]

#include <mdspan/mdspan.hpp> will pull in the reference implementation of mdspan. That doesn't use the Standard feature test macros. Thus, this part of the code would never get tested with the current CI workflow.

In fact, this code is not well formed, because layout_left_padded and layout_right_padded are class templates that take a size_t constant template parameter, unlike layout_left and layout_right that are ordinary class types without a template parameter.

Here's one way to fix this.

1. Given that we've only ever tested with the reference mdspan implementation, we could simply remove the macro test.
2. Define is_padded_layout_v traits for any specializations of layout_left_padded and layout_right_padded.
3. Use is_padded_layout_v here.

It would take some header file and macro refactoring for the reference std::linalg implementation to be able to use the actual <mdspan> header from an implementation like GCC's or Clang's.

[georgemalerbo]

Done in 110831f3: drop the __cpp_lib_submdspan test, add an is_padded_layout_v trait that matches any specialization of layout_left_padded and layout_right_padded, and use that instead.

I think __cpp_lib_aligned_accessor has the same problem
If aligned_accessor is used without std::, the name is looked up in Kokkos and not std so reference mdspan has no aligned_accessor so blas_helpers.hpp breaks and <experimental/linalg> is unincludable

So also removed __cpp_lib_aligned_accessor , is_aligned_accessor_v and its term in is_blas_accessor_type_v.

Build 6a94735 with -D__cpp_lib_aligned_accessor=202411L :

In file included from include/experimental/__p1673_bits/blas1_scale.hpp:21,
                 from include/experimental/linalg:40,
                 from tests/native/./gtest_fixtures.hpp:31,
                 from tests/native/scale.cpp:1:
include/experimental/__p1673_bits/blas_helpers.hpp:77:38: error: ‘aligned_accessor’ was not declared in this scope; did you mean ‘is_aligned_accessor_v’?
   77 | constexpr bool is_aligned_accessor_v<aligned_accessor<ElementType, ByteAlignment>> = true;
      |                                      ^~~~~~~~~~~~~~~~
      |                                      is_aligned_accessor_v
include/experimental/__p1673_bits/blas_helpers.hpp:77:16: error: parse error in template argument list
   77 | constexpr bool is_aligned_accessor_v<aligned_accessor<ElementType, ByteAlignment>> = true;
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/experimental/__p1673_bits/blas_helpers.hpp:77:16: error: wrong number of template arguments (2, should be 1)
include/experimental/__p1673_bits/blas_helpers.hpp:73:16: note: provided for ‘template<class Accessor> constexpr const bool Kokkos::Experimental::__p1673_version_0::linalg::impl::is_aligned_accessor_v<Accessor>’
   73 | constexpr bool is_aligned_accessor_v = false;
      |                ^~~~~~~~~~~~~~~~~~~~~

[mhoemmen]

Suggested change

	// Return true if the BLAS call was successfull, false otherwise.
	// Return true if a BLAS routine could be called to scale the vector, false otherwise.

The comment suggests that the BLAS could be called unsuccessfully, but the BLAS functions that could be called here don't have a failure mode.

[georgemalerbo]

Done in 1e6c8816

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 81.69014% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.15%. Comparing base (751359c) to head (874710c).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
include/experimental/__p1673_bits/blas1_scale.hpp	27.77%	12 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #334      +/-   ##
==========================================
- Coverage   97.36%   97.15%   -0.21%     
==========================================
  Files          70       70              
  Lines        5276     5347      +71     
  Branches      561      569       +8     
==========================================
+ Hits         5137     5195      +58     
- Misses        116      128      +12     
- Partials       23       24       +1     

Flag	Coverage Δ
cxx17	97.15% <81.69%> (-0.21%)	⬇️
cxx20	97.15% <81.69%> (-0.21%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[georgemalerbo]

I've been working through the changes you requested, but ended up pushing more to my fork's branch than I meant to, some of it was experimentation that really belongs on a separate branch. I've force pushed the branch back to 6a94735, the commit you reviewed.

[mhoemmen]

@georgemalerbo wrote:

I've been working through the changes you requested, but ended up pushing more to my fork's branch than I meant to, some of it was experimentation that really belongs on a separate branch. I've force pushed the branch back to 6a94735, the commit you reviewed.

Thanks for working on this! Did you have any questions about the requested changes?

[georgemalerbo]

• 110831f3 drops the __cpp_lib_submdspan test and adds is_padded_layout_v

• 88d7d1fc adds the padded layout and layout_stride tests, ungated

• 1e6c8816 applies your wording for the try_blas_scale comment

• fc92d10d - Allow parent project to set LINALG_ options and cache variables

Rather than raise the minimum cmake we can keep it at 3.12 and opt into CMP0077 and CMP0126, so a user of stdBLAS with a version of cmake 3.21 or newer can configure their project easily:

set(LINALG_ENABLE_BLAS ON)
set(LINALG_CXX_STANDARD 20)
add_subdirectory(stdBLAS)

• 2c7bb8d9 - Export the generated linalg_config.h from the linalg target

include_directories() covered the project's own tests and examples, but not a user/parent target and the linalg target exported only the source include directory

• feb8802f - Link mdspan by its exported target name and require it from the installed package

target_link_libraries(linalg INTERFACE mdspan) worked as long as mdspan came from FetchContent with its plain mdspan target and but breaks if someone tries to use an installed mdspan via find_package(mdspan) not from the in tree source build that FetchContent populates where the package config exports mdspan::mdspan instead

• 6827f449 - Include mdspan from experimental/linalg

<experimental/linalg> opens MDSPAN_IMPL_STANDARD_NAMESPACE without including <mdspan/mdspan.hpp> so it only compiles when something else included mdspan first which the tests do but a user who includes it on its own does not

A user doing the ordinary thing against the tree as it was before this commit:

cmake_minimum_required(VERSION 3.12)
project(app CXX)

add_subdirectory(stdBLAS)

add_executable(app main.cpp)
target_link_libraries(app linalg)

// main.cpp
#include <experimental/linalg>

int main() {}

$ cmake -S . -B build && cmake --build build
In file included from stdBLAS/include/experimental/linalg:35,
                 from main.cpp:1:
stdBLAS/include/experimental/__p1673_bits/blas1_givens.hpp:81:16: error: ‘is_inline_exec_v’ is not a member of ‘Kokkos::Experimental::__p1673_version_0::linalg::impl’
   81 |     && ! impl::is_inline_exec_v<Exec>
      |                ^~~~~~~~~~~~~~~~

and with this commit in, the same project unchanged:

$ cmake -S . -B build && cmake --build build
-- The CXX compiler identification is GNU 13.3.0
-- Detected support for C++23 standard
-- Using "-fconcepts" to enable concepts support
-- No installed mdspan found, fetching from Github
-- Build include directory: build/stdBLAS/include/experimental
-- Configuring done (2.9s)
-- Generating done (0.0s)
-- Build files have been written to: build
[ 50%] Building CXX object CMakeFiles/app.dir/main.cpp.o
[100%] Linking CXX executable app
[100%] Built target app

• fc12c109 - Define LINALG_HAS_CBLAS in linalg_config.h

• adds #cmakedefine LINALG_HAS_CBLAS to linalg_config.h.in

• includes that header where the guard is tested

• tests LINALG_HAS_CBLAS in place of KOKKOS_ENABLE_CBLAS

LINALG_ENABLE_BLAS looks settled already so I kept it. Renaming it to LINALG_ENABLE_CBLAS was my first thought since calling CBLAS is what it actually enables. I went with a HAS name for the internal macro rather than another ENABLE because LINALG_ENABLE_BLAS is what the user asks for and LINALG_HAS_CBLAS is what the build got

• f92109ce Find and link a BLAS behind LINALG_ENABLE_BLAS

• keeps whatever find_package(BLAS) reports instead of overwriting it

• makes LINALG_ENABLE_BLAS explicit

• carries the BLAS on the linalg target as an INTERFACE requirement, where before it reached the tests only and not the linalg itself so no one using library got a BLAS to link against

A user who adds add_subdirectory(stdBLAS) and asks for nothing now gets LINALG_ENABLE_BLAS:BOOL=OFF instead of ON. A BLAS is still found by find_package(BLAS) the presence of a cblas.h just no longer decides anything.

• 4b500ff0 Let a user name the CBLAS header when the provider does not call it cblas.h

• adds LINALG_CBLAS_HEADER and LINALG_CBLAS_INCLUDE_DIRS cache variables

• carries the chosen name into linalg_config.h as the #include argument, where before it was hardcoded to cblas.h, so every file that includes stdBLAS uses the header that was named

• 1786baf0 Document LINALG_ENABLE_BLAS

States that what it enables is calling CBLAS, and that the header can be named when the provider does not call it cblas.h.

• 4189f5eb Build and test the CBLAS path in CI

• installs libopenblas-dev, turns LINALG_ENABLE_BLAS on and runs the tests at C++17 and C++20

• asserts the generated linalg_config.h contains #define LINALG_HAS_CBLAS

• asserts the test binary has an undefined cblas_dscal to resolve

[mhoemmen]

@georgemalerbo The original title of this PR was "Fix stride(0) ordering, bound incx, reject incx == 0 and deduce the CBLAS index type." You've done that, no? Why not call that victory, get the PR merged, and create a new PR with the CI changes? What motivates combining those previous changes with the CI changes into a single PR?

Reviewers need to be able to understand what's going on. It's easier to do that if each PR fixes one thing.