VE3NEA / enigma-cuda

11 stars 5 forks source link

Fixed bug in FindBestResultKernel #4

Closed matszpk closed 7 years ago

matszpk commented 7 years ago

I found bug in FindBestResultKernel. Due to accessing beyond array bounds and wrong element indexing after first call in GetBestResult this kernel can get wrong unexpected results. I apply similar patch to my OpenCL version.

matszpk commented 7 years ago

Hi. I would like to you verify and test that changes in this kernel. I found sample case, when current version of the algorithm doesn't work and it give wrong result. I can send you that sample data. If it possible review my pull request and push to your repository.

VE3NEA commented 7 years ago

Please send me the sample case. Also, please note that the SelectHigherScore0 function does not need to be declared in the .cuh file because it is never called outside of the unit where it is defined.

matszpk commented 7 years ago

To your email address?

matszpk commented 7 years ago

I explain why an indexing in this kernel is wrong: just in next iteration (call) of this kernel, source data holds already reduced Results. However, this data hold original indices (g_idata[best_pair.index]) which refers to original data but not to already reduced. Because, only small amount of the original results will be overwritten (by copying to h_task.results) by reduced results, hence the results are correct at most cases. Also, in kernel index in temporary result best_pair is not initialized for threads beyond count threads, however that index in that threads will be used later in kernel. This patch fixes described issues.

VE3NEA commented 7 years ago

You were right, the old indices were used after the first run of FindBestResultKernel. Thank you for reporting the bug! However, this could be fixed without adding any new functions, just by setting the index at the beginning of FindBestResultKernel instead of doing that in ClimbKernel. Here is the fix and an updated unit test:

https://github.com/VE3NEA/enigma-cuda/commit/8046790e39e2a50e430d01be6cc11036f12845ec

VE3NEA commented 7 years ago

Actually that was not the only problem. All are fixed now: https://github.com/VE3NEA/enigma-cuda/commit/acbd9948a390e8d184c964fcff259be0a28e7f33