fix: SpMatrix::prepare_comm in case of no communication necessary#4844
Conversation
Currently, when computing matrix vector multiplication on the CPU, the `amrex::SpMV` method accesses memory of the input vector wherever the matrix has non zero column indices. This can lead to errors since accessing out of bounds memory is UB and can effectively lead to wrong results. This commit proposes first a fix checking if the column value is in the correct range. This commit also proposes to put the OpenMP loop one loop higher to avoid excessive thread spawning.
Test added assembles a simple algebraic system. Test fails without previous commit and passes with it.
This commit reverts the previous fix to SpMV and targets instead the `SpMatrix::prepare_comm` method instead. In the case where the problem is distributed but the non-zeros are local to each process, the previous implementation did not shift the column values leading to accessing unallocated memory. This fix introduces an else clause that simply shifts the values of the columns in this scenario similar to what happens when the non-zeros are separated into local and remote parts. This commit allows both the new `SpMv` test and the existing `GMRES` case to pass.
|
@WeiqunZhang please review this second iteration when you have the time. The |
|
Yes, I believe this is the correct fix. There is a minor issue in the test setup. The row offset vector has nrows+1 elements. We need to set the last element too. So I fixed the test and I also fused the two ParallelFor loops. |
|
/run-hpsf-gitlab-ci |
|
Actually we don't even need to set row offsets because they are already set correctly in the constructor of SpMatrix. |
|
/run-hpsf-gitlab-ci |
This is why the HPSF Gitlab CI failed to run in #4844.
|
/run-hpsf-gitlab-ci |
|
GitLab CI 1344715 finished with status: failed. See details at https://gitlab.spack.io/amrex/amrex/-/pipelines/1344715. |
|
Thanks for your help @WeiqunZhang! And thanks for merging |
2 participants
Summary
Currently, when computing matrix vector multiplication on the CPU, the
amrex::SpMVmethod can accesses memory of the input vector wherever the matrix has non zero column indices if the matrix is distributed but does not need any communication to do the multiplication. This can lead to errors since accessing out of bounds memory is UB and can effectively lead to completely wrong results.This contribution proposes first a fix by adding an else clause for the case where there are only local non-zero values in the matrix.
This commit also proposes to put the OpenMP loop one loop higher to avoid excessive thread spawning at this local matrix multiplication.
Additional background
Checklist
The proposed changes: