From 5dd4ef29dde8872bb4280328113e6f8b9254f5e8 Mon Sep 17 00:00:00 2001 From: Scott Gasch Date: Fri, 28 Aug 2026 11:46:18 -0700 Subject: Fix RescoreMovesViaSearch's fail-high handling: stop destroying the winning move's own score, and stop discarding generate.c's ordering information for moves it never got to search. Two bugs, found while reading this function to understand it: 1. On fail-high, `goto end` jumped past the for loop's own x++, so x at the `end:` label still pointed at the move that just failed high -- whose iValue had just been correctly set to its real score two lines earlier. The clearing loop then started at that same x, immediately overwriting the winning move's own just-computed score with -INFINITY: exactly backwards, marking the one move IID found good enough to fail high on as worst-possible, while the inferior moves it beat kept their real scores and would be preferred instead. 2. Even with that fixed, every move after the winner was still set to -INFINITY -- total, deliberate amnesia about generate.c's original ordering estimate for moves we simply didn't get to (a fail-high means we stop early on purpose, to avoid burning nodes confirming what we've already decided to play). If the winner's fail-high doesn't hold up at full depth, the caller falls back to a list where every remaining move is a tied -INFINITY -- worse than never having run IID at all for that tail, and inconsistent with -INFINITY's use elsewhere in this function for genuinely-known-illegal moves. Restructured to defer committing to mvf[].iValue until it's known whether every move got an honest, fully-searched score (scores go into a local scratch array during the loop instead of directly into the move stack). On full completion, commit all of them and set fMovesRescoredByIID as before. On fail-high, commit nothing -- leave every move's original generate.c ordering value untouched, and bump just the winning move into killer-tier territory (same trick generate.c uses for a real killer move) so the normal, non-rescored selection path still tries it first. fMovesRescoredByIID stays FALSE in this case, since the ply's iValue is back to being generate.c's ordering encoding, not real scores. Measured (ecm_ringers.ep_/ecm_confident_quick.ep_/ecm_hard_quick.ep_, sn=5M): 10/88/9, recovering the confident_quick point lost by the previous IID-trust commit (was 10/87/9) with no cost elsewhere. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01YGSMkwjqiCk4XhbfN7ugD2 --- src/searchsup.c | 83 +++++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 57 insertions(+), 26 deletions(-) (limited to 'src') diff --git a/src/searchsup.c b/src/searchsup.c index 8bc74ef..6f0d5f9 100644 --- a/src/searchsup.c +++ b/src/searchsup.c @@ -611,11 +611,13 @@ Return value: **/ { - ULONG x; + ULONG x, uBegin, uEnd; MOVE mv; SCORE iScore; SCORE iBestScore = -INFINITY; ULONG uBest; + FLAG fFailHigh = FALSE; + SCORE rgScores[MAX_MOVES_PER_PLY]; // // Preconditions @@ -643,11 +645,20 @@ Return value: ASSERT(uDepth < MAX_DEPTH_PER_SEARCH); // - // Search the moves and remember the scores + // Search the moves and remember the scores in a scratch array, NOT + // directly into mvf[].iValue -- we don't yet know if we'll get an + // honest score for every move at this ply (a fail-high partway + // through means we stop early, deliberately, without searching the + // rest). Committing partial data into iValue and then patching the + // gaps with a sentinel (this function used to write -INFINITY into + // every unsearched move) makes the whole ply look "rescored" to + // downstream code when it isn't really -- see the fail-high handling + // below for why that matters. // - for (x = uBest = ctx->sMoveStack.uBegin[ctx->uPly]; - x < ctx->sMoveStack.uEnd[ctx->uPly]; - x++) + uBegin = ctx->sMoveStack.uBegin[ctx->uPly]; + uEnd = ctx->sMoveStack.uEnd[ctx->uPly]; + ASSERT((uEnd - uBegin) <= MAX_MOVES_PER_PLY); + for (x = uBest = uBegin; x < uEnd; x++) { SelectBestNoHistory(ctx, x); mv = ctx->sMoveStack.mvf[x].mv; @@ -667,40 +678,60 @@ Return value: } } UnmakeMove(ctx, mv); - ctx->sMoveStack.mvf[x].iValue = iScore; + rgScores[x - uBegin] = iScore; if (iScore > iBestScore) { uBest = x; iBestScore = iScore; if (iScore >= iBeta) { - goto end; + fFailHigh = TRUE; + break; } } } + else + { + // Genuinely illegal (pseudo-legal but leaves king in check) + // -- this isn't "unknown," we know for a fact it's worthless + // and the caller will skip it too when MakeMove fails again. + rgScores[x - uBegin] = -INFINITY; + } } - end: - // - // Clear the values from the current move to the end of the - // moves in the list so that if we failed high we don't have - // stale move values in the list for moves we did not search - // the subtrees for. - // - while(x < ctx->sMoveStack.uEnd[ctx->uPly]) + if (FALSE == fFailHigh) + { + // We have an honest, fully-searched score for every move at + // this ply -- commit them all and let downstream code + // (ComputeMoveScore, the main search loop's move-selection + // call) trust iValue as a real eval-axis score. + for (x = uBegin; x < uEnd; x++) + { + ctx->sMoveStack.mvf[x].iValue = rgScores[x - uBegin]; + } + ctx->sPlyInfo[ctx->uPly].fMovesRescoredByIID = TRUE; + } + else { - ctx->sMoveStack.mvf[x].iValue = -INFINITY; - x++; + // We only know one honest thing: uBest is good enough to have + // failed high at reduced depth. We do NOT have honest scores + // for the rest (deliberately -- searching them would burn nodes + // solely to confirm what we've already decided to do, which is + // play uBest). Rather than inventing eval-axis values for moves + // we never examined (the old code wrote -INFINITY into all of + // them, including -- until fixed -- overwriting uBest's own + // just-computed score), leave every move's original generate.c + // ordering value completely untouched and just bump uBest into + // killer-tier territory so the normal (non-rescored) selection + // path -- SelectBestWithHistory/NoHistory -- picks it first, the + // same trick generate.c uses for a real killer move. Do NOT set + // fMovesRescoredByIID: this ply's iValue is back to being + // generate.c's ordering encoding, not real scores, for every + // move including uBest (whose bump is itself just an ordering + // flag, not part of its own eval-axis value). + ctx->sMoveStack.mvf[uBest].iValue |= FIRST_KILLER; } - // uBest already holds the largest real score in the list (iBestScore - // tracked the running max as we went) -- SelectBest{With,No}History - // just compare raw magnitude, so it naturally sorts first without - // needing a flag. OR-ing in SORT_THESE_FIRST here used to corrupt - // that real score into looking like generate.c's biased-capture- - // ordering format to any later ComputeMoveScore() caller; removed. - // fMovesRescoredByIID (below) is the correct, non-destructive way to - // signal "trust this ply's iValue as a real eval-axis score." - ctx->sPlyInfo[ctx->uPly].fMovesRescoredByIID = TRUE; + ASSERT(IS_VALID_SCORE(iBestScore)); return(iBestScore); } -- cgit v1.3