Fix stride(0) ordering, bound incx, reject incx == 0 and deduce the CBLAS index type - #334
Fix stride(0) ordering, bound incx, reject incx == 0 and deduce the CBLAS index type#334georgemalerbo wants to merge 5 commits into
Conversation
This is for discussion of issue kokkos#328.
mhoemmen
left a comment
There was a problem hiding this comment.
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!
| // AMK 5/20/26 - layout_left_padded and layout_right_padded were added in C++26. | ||
| // According to cppreference, padded layouts are covered by the same feature test as submdspan. | ||
| #ifdef __cpp_lib_submdspan | ||
| std::is_same_v<Layout, layout_left_padded> || | ||
| std::is_same_v<Layout, layout_right_padded> || | ||
| #endif |
There was a problem hiding this comment.
#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.
- Given that we've only ever tested with the reference mdspan implementation, we could simply remove the macro test.
- Define
is_padded_layout_vtraits for any specializations oflayout_left_paddedandlayout_right_padded. - Use
is_padded_layout_vhere.
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.
| return blas_value_type && blas_layout && blas_accessor; | ||
| } | ||
|
|
||
| // Return true if the BLAS call was successfull, false otherwise. |
There was a problem hiding this comment.
| // 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.
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