Skip to content

Fix stride(0) ordering, bound incx, reject incx == 0 and deduce the CBLAS index type - #334

Open
georgemalerbo wants to merge 5 commits into
kokkos:mainfrom
georgemalerbo:wrap_scal_review_fixes
Open

Fix stride(0) ordering, bound incx, reject incx == 0 and deduce the CBLAS index type#334
georgemalerbo wants to merge 5 commits into
kokkos:mainfrom
georgemalerbo:wrap_scal_review_fixes

Conversation

@georgemalerbo

Copy link
Copy Markdown

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 mhoemmen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Comment on lines +50 to +55
// 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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.

return blas_value_type && blas_layout && blas_accessor;
}

// Return true if the BLAS call was successfull, false otherwise.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

3 participants