DenseMatrix is incompatible with LAPACK calls
Hi Klaus,
At some point DenseMatrix started adding padding without giving any user control over whether or not that is done. This makes Blaze incompatible with LAPACK calls, which is a big pain for us (and presumably other people). Would it be reasonable to fix this? We have found an workaround where we change to column major, and then change all the LAPACK calls in the entire code base. Nevertheless, being able to use Blaze more easily with LAPACK would be nice.
Also, since the padding was never specified as part of the API, I guess it’s arguable whether or not this counts as an API change, but it certainly is in a practical sense. Would it maybe be possible to leave such breaking changes to major version bumps?
Thanks!
Best,
Nils
Comments (9)
-
-
Hi Klaus,
Thanks for your reply. Nils was describing some trouble that we had when we recently moved to supporting Blaze 3.7 in our codebase.
You’re correct that we are referring specifically to
DynamicMatrix
, which to the best of our understanding does not support adjusting the padding policy via a template argument. I think it is true that in principal the API specification made no guarantee for the padding policy for earlier versions, but our experience appears to indicate that the actual data layout changed nontrivially between 3.6 and 3.7, at least on certain machines we tested on.Previously, we had a custom matrix class that inherits from the Blaze
DynamicMatrix
as:template <typename T, bool SO = blaze::defaultStorageOrder> class DenseMatrix : public blaze::DynamicMatrix<T, SO>
And our old LAPACK calls were of the flavor:
dgesv_(&rhs_vector_size, &number_of_rhs, matrix_operator->data(), &output_vector_size, pivots->data(), rhs_in_solution_out->data(), &output_vector_size, &info);
where the
matrix_operator
is the derived class ofDynamicMatrix
- note that previously using theLDA
argument of LAPACK as simply the extent of the matrix was sufficient. This particular use-case no longer functioned when including Blaze 3.7 instead.The alteration to this set to get back to a working linear algebra system was to first move to
template <typename T, bool SO = blaze::columnMajor> class DenseMatrix : public blaze::DynamicMatrix<T, SO>
and to adjust the LAPACK call to privide
matrix_operator->spacing()
to the LDA parameter (and similar changes for several other calls). It was my understanding that the layout assumptions of LAPACK calls would not have sufficient stride options to support the row-major padding fromblaze:defaultStorageOrder
- Is that mistaken?Thanks for your help in getting to the bottom of this!
Cheers
Jordan
-
Hi Jordan!
Thanks for the additional explanation. By now I have a suspicion what could cause a problem: Could you please check if you perhaps on the
BLAZE_USE_PADDING
compilation switch?Best regards,
Klaus!
-
Hi Klaus,
I’ve tried the example in question again with the compiler switch
#define BLAZE_USE_PADDING 0
set in our configuration header, and confirmed with tracing output that it is indeed set to zero, but the derived class of
DynamicMatrix
still exhibits padding when we access the memory layout directly via thedata
member function. The size of the matrix columns (still imposingblaze::columnMajor
in this test) appears to be padded out to an integer multiple of 4 on my machine.Thanks again!
Jordan
-
Hi Jordan!
Thanks, that confirms my suspicion. Unfortunately the
BLAZE_USE_PADDING
flag is no longer used and was supposed to be replaced by an instance specific configuration. However, I didn’t realize that now the state of affairs is caught between issue#134, which made it into Blaze 3.7 and issue#307, which didn’t quite make it. In summary, the error is on my side and I will reactivate the flag within the next 24 hours. I apologize for the inconvenience and your lost time!Still, I will mark the
BLAZE_USE_PADDING
flag as deprecated, since the decision to have a global padding configuration was not a good one to begin with. I will keep the flag for the configuration ofDynamicVector
andDynamicMatrix
until at least Blaze 3.8, but afterwards remove it. By then you’ll have to update your LAPACK calls or to use the new instance-specific configuration options.Thanks a lot for pointing out this defect,
Best regards,
Klaus!
-
-
assigned issue to
-
assigned issue to
-
- changed status to open
-
- changed status to resolved
-
Hi Klaus,
Excellent, thank you for your quick attention to this issue!
Much obliged,
Jordan
- Log in to comment
Hi Nils!
Thanks for creating this issue. I agree that silently adding padding would be a breaking API change and would be a change reserved for major version bumps. However, there hasn’t been a breaking API change with regards to padding, though, and from my point of view there is no incompatibility with LAPACK. Allow me to explain:
DenseMatrix
is a CRTP base class for all kinds of dense matrix (see the wiki), i.e.DenseMatrix
does not have anything to do with padding. Do you meanDynamicMatrix
?StaticMatrix
,HybridMatrix
, andDynamicMatrix
), use padding since version 1.0. Padding is the reason why the interface of dense matrices doesn’t provide a begin() and end() function for the traversal of the entire matrix (see our discussion in issue #178).StaticMatrix
to disable (!) padding (see the wiki). The interface was merely extended, though, and no defaults were changed.I hope the explanation helps to resolve a possible misunderstanding. If not, could you please provide me with a minimum example where and how Blaze is not compatible to LAPACK? That would help me to get a better understanding of the real issue. Thanks again,
Best regards,
Klaus!