Skip to content
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

trmm() refactoring #39

Merged
merged 5 commits into from
Oct 5, 2024
Merged

trmm() refactoring #39

merged 5 commits into from
Oct 5, 2024

Conversation

mkatliar
Copy link
Owner

@mkatliar mkatliar commented Sep 30, 2024

No more trmmLeftUpper() and trmmRightLowe(), just trmm(). There are overloads for left and right multiplication differing by the argument order, and there is the uplo argument specifying whether the upper or lower-triangular part should be used.

@mkatliar mkatliar force-pushed the trmm branch 3 times, most recently from 682b334 to 9b58ce3 Compare October 1, 2024 07:51
@mkatliar mkatliar requested a review from roversch October 1, 2024 07:53
Copy link
Collaborator

@roversch roversch left a comment

Choose a reason for hiding this comment

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

Small comments, please address

include/blast/math/algorithm/Trmm.hpp Outdated Show resolved Hide resolved
include/blast/math/algorithm/Trmm.hpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@roversch roversch left a comment

Choose a reason for hiding this comment

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

This MR looks good.

However, one alternative design would encode the UpLo property in the matrix type, either via its own class or a trait. Same for UnitDiagonal.

Any reason why you didn't go that way?

auto au = ~a;

if constexpr (SO == columnMajor)
{
#pragma unroll
for (size_t k = 0; k < N; ++k)
for (size_t k = 0; k < N; ++k) if (k < n)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Formatting

Copy link
Owner Author

Choose a reason for hiding this comment

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

This is intentional. If is my way to write an unrolled loop from 0 to n.

for (size_t i = 0; i < A.rows(); ++i)
for (size_t j = i + 1; j < A.columns(); ++j)
blaze::reset(A(i, j));
reference::trmm(alpha, B, A, UpLo::Lower, false, C);

// TODO: should be strictly equal?
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can always use integer calculations, if you want strict equality (floating point arithmetic on integers is accurate)

@mkatliar
Copy link
Owner Author

mkatliar commented Oct 5, 2024

This MR looks good.

However, one alternative design would encode the UpLo property in the matrix type, either via its own class or a trait. Same for UnitDiagonal.

Any reason why you didn't go that way?

I did not want to go that way because I wanted to keep the API close to BLAS. Also, encoding the "upper-lower triangular" and "unit diagonal" properties in the matrix type would complicate the type system, and I don't see a big advantage of it. This is what Blaze does, and if you look at Blaze you see how much it complicates the implementation.

@mkatliar mkatliar merged commit 0daa999 into master Oct 5, 2024
2 checks passed
@mkatliar mkatliar deleted the trmm branch October 5, 2024 10:38
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