[REVIEW] fixed an index issue in bloom_filter_impl #675
Conversation
eleanorTurintech
requested review from
PointKernel and
sleeepyjack
as code owners
PointKernel
added
type: bug
|
/ok to test |
PointKernel
left a comment
PointKernel
left a comment
There was a problem hiding this comment.
We probably need stricter unit tests for the CG add.
@eleanorTurintech Thanks a lot for catching and fixing those issues!
sleeepyjack
left a comment
sleeepyjack
left a comment
There was a problem hiding this comment.
Thank you @eleanorTurintech for this contribution!
I reworked the CG add in #672 a couple of days ago but haven't merged the changes yet. Let me get that merged first and then we can evaluate which of your changes are still relevant.
| auto const word = policy_.word_pattern(hash_value, rank); | ||
| auto const word = policy_.word_pattern(hash_value, i); | ||
|
|
||
| auto atom_word = | ||
| cuda::atomic_ref<word_type, thread_scope>{*(words_ + (idx * words_per_block + rank))}; | ||
| cuda::atomic_ref<word_type, thread_scope>{*(words_ + (idx * words_per_block + i))}; |
There was a problem hiding this comment.
Ha, very good catch! 🎣 The bug would have surfaced if we use a CG that is smaller than the optimal CG size.
|
@eleanorTurintech #672 has just been merged. The fix for |
You are right. The |
|
/ok to test |
eleanorTurintech
changed the title
3 participants
There seem to be some small issues with the for-loop indices in bloom filter. This pr aims to fix them.
In particular, there are two issues that this pr fixes:
cuco::detail::bloom_filter_impl::add(CG const& group, ProbeKey const& key), variableiin the for-loop doesn't seem to be used for indexingwordandatom_word. This pr updates the index fromranktoi.cuco::detail::bloom_filter_impl::clear(CG const& group), the for-loop's terminating condition was not complete. This pr addsi <to the condition to complete it.