Decouple from Blaze - part 2 #38
Conversation
Add IsStatic specialization for const types
mkatliar
changed the title
| * @throw @a std::invalid_argument if non-square matrix is provided | ||
| */ | ||
| template <Matrix MT> | ||
| inline void makePositiveDefinite(MT& matrix) |
There was a problem hiding this comment.
make in C++ often refers to making a new object (make_unique etc.)
better name: fillPositiveDefinite?
There was a problem hiding this comment.
This name was borrowed from Blaze, and there is already code that uses makePositiveDefinite(). Also it is possible to make an already existing matrix positive-definite, so the name seems to make sense.
| , spacing_ {nextMultiple(SO == columnMajor ? m : n, SimdSize_v<T>)} | ||
| // Initialize padding elements to 0 to prevent denorms in calculations. | ||
| // Denorms can significantly impair performance, see https://github.com/giaf/blasfeo/issues/103 | ||
| , v_ {new(std::align_val_t {alignment_}) T[spacing_ * (SO == columnMajor ? n : m)] {}} |
There was a problem hiding this comment.
Why not introduce capacity_ as in static matrix?
There was a problem hiding this comment.
In DynamicMatrix, capacity is not known at compile-time. So it would be an extra field taking memory but not used.
| { | ||
| ET v {}; | ||
| for (size_t k = 0; k < K; ++k) | ||
| v += *(~A)(i, k) * *(~B)(k, j); |
There was a problem hiding this comment.
What a horrible syntax 😛
There was a problem hiding this comment.
Somewhat ugly, yes. The ~ makes any matrix pointer unaligned, and * is the dereferencing.
Any suggestions on how to improve the syntax?
| * @tparam AF whether the submatrix lop left element is aligned | ||
| */ | ||
| template <Matrix MT, AlignmentFlag AF> | ||
| class Submatrix |
There was a problem hiding this comment.
Can you explain in the comments why mdspan wasn't used as an implementation?
There was a problem hiding this comment.
I did not used it because:
- it does not know whether it is aligned or not, which is a blocker
- everywhere in the code matrix elements are accessed with
(), andstd::mdspanuses[]. It would require changes all over the code to make it work withstd::mdspan - I wanted to have more control over the implementation, in case there is or will be something that
std::mdspanlacks
| @@ -0,0 +1,18 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
Does __restrict really make a difference? Or do you do it because Blaze had it as well?
There was a problem hiding this comment.
I did not check, but it might. I think Blaze uses it for a reason.
The primary goal was to make the
RegisterMatrixTest.testPotrfwork on ARM. The Blaze matrices had to be replaced by BLAST matrices, and multiple other changes were required to achieve this.This PR is big, but it is difficult to split it into smaller ones, because the changes are not independent. Summary of the changes:
RegisterMatrixTest.testPotrfworksgemm()implementationRegisterMatrixTestDynamicMatrixclasstrans(),submatrix(),randomize(),makePositiveDefinite()implemented for BLAST matricesRandomize.hppmoved toblast/math/algorithm.