-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Description
I'm not really sure what the correct procedure is for bug fixes. The testing guidelines tell me to discuss it in the forum, but the forum doesn't seem to be the right place anymore.
do_null_move() copies the accumulator from the previous state into the new state, which is correct. It then clears the "computed_score" flag because the side to move has changed, and with the other side to move NNUE will return a completely different evaluation (normally with changed sign but also with different NNUE-internal tempo bonus).
The problem is that do_null_move() clears the wrong flag. It clears the computed_score flag of the old state, not of the new state.
It turns out that this almost never affects the search. For example, fixing it does not change the current bench (but it does change the previous bench).
If others agree that this is a bug, I can start a non-regression test on the following fix:
syzygy1/Stockfish@503fa01
It would also be possible to simplify the bug away by removing the computed_score flag. SF never uses it (or at least never in a correct manner). Where SF needs the evaluation of the current position multiple times, it already keeps it in a local variable.
Lastly, it should be possible to speed up do_null_move() a little by copying the previous accumulator only when it is useful (when computed_accumulator is set). I tried this a while ago but (to my surprise) could not detect a clear speed up, so I am not convinced that it would pass as an improvement (I did not try it in fishtest). I could combine this small probable improvement with the bug fix in a non-regression test, but that would be kind of cheating. (Any ideas?)