-
Notifications
You must be signed in to change notification settings - Fork 707
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
Fix integer overflow in TensorProductMatrix #5911
Conversation
/run-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from a conceptual point of view, but the part that finds the run-time size is a bit too complicated in my opinion, see the comments below.
} | ||
return n_rows_max; | ||
}; | ||
const unsigned int nm_flat_size_max = n_rows_max() * n_rows_max() * macro_size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not make this a lambda (without argument) since you just count the components. You can simply do this inline for the variable n_rows_max
.
std::array<std::size_t,dim> n_rows; | ||
for (unsigned int d = 0; d < dim; ++d) | ||
n_rows[d] = mass_matrix[d].n_rows(); | ||
n_rows_max = *(std::max_element (n_rows.cbegin(),n_rows.cend())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks too complicated. Why not simply do the computation as
std::size_t n_rows_max = 0;
for (unsigned int d=0; d<dim; ++d)
n_rows_max = std::max(n_rows_max, mass_matrix[d].n_rows();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks! In addition, I replaced the reserve
with resize
and deleted redundant resize
s inside the loop, since vectorized_transpose_and_store
has the control over the actual data size.
a4a8fc3
to
c1da6eb
Compare
c1da6eb
to
c42a128
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jwitte08
I have encountered an integer overflow for a 3D use case with compile-time constant size=144. The guilty part causing the overflow is
fixed_int_power
, where the guilty part is actually me!What we really want to do is to determine the maximum row-size of all directional mass matrices and use this to reserve memory for our (intermediate) flat data arrays. This avoids a re-allocation within the loop at line 758. Mistakenly, I took the
dim
th power of the row-sizes.In my opinion, it makes sense to avoid a re-allocation, but maybe it is just overly complicated and one should just do the
resize
s in the loop. Note that a re-allocation would only appear in the case where we have a non-isotropic Kronecker decomposition.I am open to any suggestion!