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 <[email protected]>
Claude-Session: https://claude.ai/code/session_01YGSMkwjqiCk4XhbfN7ugD2

1 files changed, 57 insertions, 26 deletions

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);
 }