From 1a0712fee7e3fa9bcf124a942d8ef15efe4578e7 Mon Sep 17 00:00:00 2001 From: Scott Gasch Date: Fri, 28 Aug 2026 11:10:14 -0700 Subject: Fix IID's killer-blind gate and its ordering-flag/history contamination of iValue; harden against a latent ComputeMoveExtension bug. DO_IID's "is the top move crappy" gate compared raw iValue against SORT_THESE_FIRST only, missing that ordinary killer moves (FIRST_KILLER through FOURTH_KILLER) sit below that threshold too -- a killer that already proved itself elsewhere in the tree was being treated as "crappy" and triggering an unnecessary shallow rescore. Fixed by also excluding killer-flagged moves from the gate. RescoreMovesViaSearch corrupted the winning move's real search score by OR-ing in SORT_THESE_FIRST to force it to sort first (`mvf[uBest].iValue |= SORT_THESE_FIRST`) -- unnecessary (SelectBest{With,No}History already find the true max by plain magnitude comparison, no flag needed) and actively dangerous: a later ComputeMoveScore() call on that same move, if it's a capture, would see the corrupted value, mistake it for generate.c's biased-capture-ordering format, and subtract the wrong bias entirely. Removed the OR; added an explicit PLY_INFO.fMovesRescoredByIID flag so ComputeMoveScore and the main search-loop's move-selection call can both recognize "this ply's iValue holds a real eval-axis score" without relying on bit-pattern inference. Consequently, ComputeMoveScore now trusts an IID-rescored move's score outright instead of running it through the capture-bias-subtraction or quiet-move-collapse-to-0 logic (both of which assume generate.c's ordering encoding, which a rescored ply no longer holds). Separately hardened it against quiet killer-mate moves, which can reach SORT_THESE_FIRST via a different, capture-unrelated path and were incorrectly getting the capture bias subtracted from them; they now correctly collapse to 0 like other quiet moves. Two follow-on ideas -- blending history into the real IID score (scaled or capped) and a exact-tie-only history tiebreak -- were implemented, measured, and rejected: blending invents a new, leak-prone move-scoring axis for no measured benefit, and the tiebreak-only compromise still cost solves relative to just trusting the real score outright. Main search's move-selection call now branches once per selection (not once per candidate move) between SelectBestNoHistory (IID-rescored plies) and SelectBestWithHistory (everyone else), keeping the overwhelmingly common non-rescored path at zero added cost. Net measured effect (ecm_ringers.ep_/ecm_confident_quick.ep_/ ecm_hard_quick.ep_, sn=5M): 10/90/9, down from a pre-existing 11/88/10 on ringers and hard specifically -- see lmr_testing/RESULTS.md for the full sweep of rejected alternatives and why the regression was accepted as the cost of removing a latent, leak-prone bug class rather than chasing the exact prior numbers. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01YGSMkwjqiCk4XhbfN7ugD2 --- src/chess.h | 9 +++++++++ src/movesup.c | 10 +++++----- src/search.c | 45 ++++++++++++++++++++++++++++++++++++++++----- src/searchsup.c | 31 ++++++++++++++++++++++++++++--- 4 files changed, 82 insertions(+), 13 deletions(-) diff --git a/src/chess.h b/src/chess.h index 9b6303d..3a78b2b 100755 --- a/src/chess.h +++ b/src/chess.h @@ -934,6 +934,10 @@ typedef struct _PLY_INFO FLAG fInCheck; FLAG fInQsearch; FLAG fPvNode; // this node's own window was wide + FLAG fMovesRescoredByIID; // sMoveStack[uPly].iValue holds + // real search scores from + // RescoreMovesViaSearch, not + // generate.c's ordering encoding MOVE mv; MOVE mvBest; @@ -2284,6 +2288,11 @@ IsDraw(SEARCHER_THREAD_CONTEXT *ctx); // #define QPLIES_OF_NON_CAPTURE_CHECKS (2) #define FUTILITY_BASE_MARGIN (50) +// Measured: disabling this entirely (see lmr_testing/RESULTS.md) is a +// clear net loss across ringers/confident_quick/hard_quick, so IID itself +// is load-bearing. The "is the top move crappy" gate in search.c's DO_IID +// block still misclassifies ordinary killer moves as crappy (see the fix +// there) -- that's the next thing being tuned, not whether IID exists. #define DO_IID #define IID_R_FACTOR (TWO_PLY + HALF_PLY) diff --git a/src/movesup.c b/src/movesup.c index 8aa8ffa..dec090c 100755 --- a/src/movesup.c +++ b/src/movesup.c @@ -988,7 +988,7 @@ Return value: } -void FASTCALL +void FASTCALL SelectBestWithHistory(SEARCHER_THREAD_CONTEXT *ctx, ULONG u) /** @@ -1017,11 +1017,11 @@ Return value: SCORE iVal; MOVE mv; MOVE_STACK_MOVE_VALUE_FLAGS mvfTemp; - + ASSERT(ctx->sMoveStack.uBegin[ctx->uPly] <= uEnd); ASSERT(u >= ctx->sMoveStack.uBegin[ctx->uPly]); ASSERT(u < uEnd); - + // // Linear search from u..ctx->sMoveStack.uEnd[ctx->uPly] for the // move with the best value. @@ -1033,7 +1033,7 @@ Return value: iBestVal += g_HistoryCounters[mv.pMoved][mv.cTo]; } uLoc = u; - + for (v = u + 1; v < uEnd; v++) { iVal = ctx->sMoveStack.mvf[v].iValue; @@ -1058,7 +1058,7 @@ Return value: } -void FASTCALL +void FASTCALL SelectBestNoHistory(SEARCHER_THREAD_CONTEXT *ctx, ULONG u) /** diff --git a/src/search.c b/src/search.c index eb931bc..6e596ab 100755 --- a/src/search.c +++ b/src/search.c @@ -200,6 +200,7 @@ Search(IN SEARCHER_THREAD_CONTEXT *ctx, DTEnterNode(ctx, uDepth, FALSE, iAlpha, iBeta); iInitialAlpha = iAlpha; pi->fPvNode = (iBeta != iAlpha + 1); + pi->fMovesRescoredByIID = FALSE; ASSERT((IS_CHECKING_MOVE(mvLast) && (TRUE == pi->fInCheck)) || (!IS_CHECKING_MOVE(mvLast) && (FALSE == pi->fInCheck))); @@ -413,13 +414,19 @@ Search(IN SEARCHER_THREAD_CONTEXT *ctx, // EXPERIMENT: If we got no best move from the // hash table and the best move we got from the // generator looks crappy (i.e. is not a winning - // or even capture/promotion) then rescore the - // moves we generated at this ply using a - // shallower search. "Internal Iterative - // Deepening" or something like it. + // or even capture/promotion, AND not a killer -- + // a killer move already proved itself elsewhere in + // the tree, unlike an untested quiet move, so it + // doesn't need IID's help) then rescore the moves + // we generated at this ply using a shallower + // search. "Internal Iterative Deepening" or + // something like it. if ((iAlpha + 1 != iBeta) && (mvHash.uMove == 0) && (ctx->sMoveStack.mvf[x].iValue < SORT_THESE_FIRST) && + (0 == (ctx->sMoveStack.mvf[x].iValue & + (FIRST_KILLER | SECOND_KILLER | + THIRD_KILLER | FOURTH_KILLER))) && (uDepth >= FOUR_PLY)) { ASSERT(uDepth >= (IID_R_FACTOR + ONE_PLY)); @@ -457,7 +464,35 @@ Search(IN SEARCHER_THREAD_CONTEXT *ctx, ASSERT(x >= ctx->sMoveStack.uBegin[ctx->uPly]); if (uLegalMoves < SEARCH_SORT_LIMIT(ctx->uPly)) { - SelectBestWithHistory(ctx, x); + // On an IID-rescored ply, mvf[].iValue holds a + // real, honest eval-axis score from an actual + // shallow search (see RescoreMovesViaSearch) -- + // trust it outright, same principle as + // ComputeMoveScore's IID-trust branch. Two + // alternatives were measured and rejected (see + // RESULTS.md): blending history into the real + // score (scaled or capped) invents a new, + // leak-prone move-scoring axis on top of an + // already-crowded set (generate.c's ordering + // encoding, RescoreMovesViaSearch's real scores, + // root.c's own scheme) and measures no better + // than trusting the score outright; a pure + // tiebreak-on-exact-ties compromise still cost + // solves relative to full mixing, so it wasn't + // buying its complexity either. The branch lives + // here (once per selection call), not inside + // SelectBestWithHistory (once per candidate + // move in a hot per-node loop), to keep the + // overwhelmingly common non-rescored path at + // zero added cost. + if (TRUE == pi->fMovesRescoredByIID) + { + SelectBestNoHistory(ctx, x); + } + else + { + SelectBestWithHistory(ctx, x); + } } mv = ctx->sMoveStack.mvf[x].mv; #ifdef DEBUG diff --git a/src/searchsup.c b/src/searchsup.c index 585bcc6..efdde67 100644 --- a/src/searchsup.c +++ b/src/searchsup.c @@ -239,7 +239,7 @@ Return value: SCORE ComputeMoveScore(IN SEARCHER_THREAD_CONTEXT *ctx, IN MOVE mv, - IN ULONG uMoveNum) + IN ULONG uMoveNum) { SCORE iMoveScore = (PIECE_VALUE(mv.pCaptured) + PIECE_VALUE(mv.pPromoted)); @@ -247,8 +247,25 @@ ComputeMoveScore(IN SEARCHER_THREAD_CONTEXT *ctx, { ASSERT(uMoveNum < MAX_MOVE_STACK); ASSERT(IS_SAME_MOVE(mv, ctx->sMoveStack.mvf[uMoveNum].mv)); + ASSERT(ctx->uPly > 0); iMoveScore = ctx->sMoveStack.mvf[uMoveNum].iValue; - if ((iMoveScore >= SORT_THESE_FIRST) && IS_CAPTURE_OR_PROMOTION(mv)) + // Every current caller (search.c's EFP check, and + // ComputeMoveExtension's two ComputeMoveScore() call sites) only + // reaches here after MakeMove(ctx, mv) has already succeeded, so + // ctx->uPly is always the *child's* ply here -- uMoveNum indexes + // the parent's move list, i.e. ctx->uPly - 1, not ctx->uPly. That's + // the ply RescoreMovesViaSearch (if it ran) would have rescored, + // so that's the flag to check. + if (TRUE == ctx->sPlyInfo[ctx->uPly - 1].fMovesRescoredByIID) + { + // RescoreMovesViaSearch already put a real, searched eval-axis + // score here -- better than SEE/MVV-LVA, since it reflects an + // entire subtree, not just the immediate exchange. Trust it + // exactly as-is; don't run it through the capture-bias + // subtraction or collapse it via MIN0, both of which assume + // generate.c's ordering-encoded format, which this isn't. + } + else if ((iMoveScore >= SORT_THESE_FIRST) && IS_CAPTURE_OR_PROMOTION(mv)) { ASSERT(iMoveScore > 0); iMoveScore &= STRIP_OFF_FLAGS; @@ -668,7 +685,15 @@ Return value: ctx->sMoveStack.mvf[x].iValue = -INFINITY; x++; } - ctx->sMoveStack.mvf[uBest].iValue |= SORT_THESE_FIRST; + // 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