fix the race condition in lu factorization #1850
Conversation
yhmtsai
added
1:ST:ready-for-review
ginkgo-bot
added
mod:cuda
yhmtsai
force-pushed
the
fix_factorization_race
branch
from
4f684b5 to
84c1bbe
Compare
| // We need to load vals after synchronize. | ||
| // the next lower_nz might be modified if the dep row has the same col | ||
| // as next lower_nz's col. | ||
| const auto val = vals[lower_nz]; |
There was a problem hiding this comment.
Not sure I follow - each warp only modifies memory locations belonging to its row, so there are only data races between threads of the same warp. So is this reordering actually necessary?
There was a problem hiding this comment.
What you essentially want to do is add another warp sync at the end of each iteration of the outer loop, to prevent the modification of previous loops racing with ready from the following loop iterations? I would prefer having that sync happen explicitly at the end of the loop than hidden inside the scheduler wait function. I don't think warp syncs should be costly.
There was a problem hiding this comment.
Exactly. It is to ensure that the modification of the same warp can be seen from the others when getting it.
Yes, it is what I mean in the PR description. We only need warp sync.
I thought the wait(dep) should also imply that because it make the memory visible in block at least.
I just move that rather than introducing the warp sync
| const auto diag = vals[diag_idx]; | ||
| // we need sync to ensure all threads get the data before assigning to | ||
| // scale. | ||
| warp.sync(); |
This PR fixes the race condition in lu factorization.
we need a sync before grabbing
val[lower_nz]because the warp can modify the entry in the previous iteration.We only need the warp sync. The wait(dep) implicitly has that if I understand it correctly, so I just move it after the wait(dep).
After move, we need another sync before assigning scale to ensure every thread gets the data before modification.