diff options
Diffstat (limited to 'src')
| -rw-r--r-- | src/board_representation/EVAL.md | 242 |
1 files changed, 233 insertions, 9 deletions
diff --git a/src/board_representation/EVAL.md b/src/board_representation/EVAL.md index 697ea38..210340e 100644 --- a/src/board_representation/EVAL.md +++ b/src/board_representation/EVAL.md @@ -1,14 +1,32 @@ # Migration plan: bitboard-backed `Eval()` (rewrite, 2026-09-04) -**Status: planning only, superseding the earlier draft of this -document in full.** That draft was written before the movegen -bitboard work (`MOVEGEN_MIGRATION.md`) landed and assumed a slider -attack-bitboard primitive ("`_RookAttacksBB(c, bbOccupied)`... already -built") that did not actually exist yet at the time. It now does -- -this rewrite is based on having read `_EvalBishop`/`_EvalKnight`/ -`_EvalRook`/`_EvalQueen` in full (not by memory/analogy) and the -landed `generate.c` bitboard infrastructure (real magic tables, not a -sketch) side by side. +**Status as of 2026-09-05: in progress, not planning-only anymore.** +Pawns, `bbOccupied`, the piece-dispatch loop, knight, and bishop are +landed and committed; rook, queen, and the final `_EvalKing`/ +`_WhoControlsSquareFast` cleanup remain. See "Progress log and lessons +learned" at the end of this document for the full, current account -- +read that section first, it supersedes some of the sequencing/toggle +assumptions below (particularly section 4's per-piece `#define` +toggle idea, which was not what actually got used). + +The rest of this document below is the original rewrite plan as +scoped before implementation started. It's kept because the technical +reduction for each piece type (section 1b) and the retirement +criteria (section 8) are still accurate and worth reading in full +before touching rook or queen -- just read the progress log first for +what's actually true about the current state of the code. + +--- + +**Original status note (2026-09-04, now historical): planning only, +superseding the earlier draft of this document in full.** That draft +was written before the movegen bitboard work (`MOVEGEN_MIGRATION.md`) +landed and assumed a slider attack-bitboard primitive +("`_RookAttacksBB(c, bbOccupied)`... already built") that did not +actually exist yet at the time. It now does -- this rewrite is based +on having read `_EvalBishop`/`_EvalKnight`/`_EvalRook`/`_EvalQueen` in +full (not by memory/analogy) and the landed `generate.c` bitboard +infrastructure (real magic tables, not a sketch) side by side. ## 0. Why this rewrite exists, and what's different now @@ -668,3 +686,209 @@ knight's likely-clean bill of health lower the bar for bishop's two-x-ray-category case or queen's combined-ray-family case -- each has different enough special-case surface (section 1b) to warrant its own full pass through this list. + +## 9. Progress log and lessons learned (2026-09-05) + +This section is the authoritative account of what has actually +happened, kept up to date as work lands. Read this before touching +rook, queen, or `_EvalKing` -- it corrects several assumptions in the +plan above (particularly section 4's toggle strategy, which is not +what was actually used) and records methodology worth repeating. + +### What's landed (commits, in order) + +- `57502d6` -- Pawns retire `bvAttacks` entirely. `pos->bbPawnAttacks[2]` + computed via shift-and-mask from `pos->bbPawns[2]` (the same + technique `generate.c`'s `_GenerateAllPawnMovesBB` already uses, + minus the enemy-occupancy mask). Two silent-regression bugs found + by auditing every remaining `|8` site by hand after the fact, not + by any test failing: `_EvalPawns`' own pawn-duo/backward-pawn + detection, and `_WhoControlsSquareFast`, both read `bvAttacks` at + points in `Eval()`'s sequence where only pawns could have written + it -- once pawns stopped writing there, both went silently wrong. + Fixed by reading `bbPawnAttacks` directly instead. +- `2ce3570` -- `pos->bbOccupied` added, incrementally maintained by + `move.c`'s existing `SlidePiece`/`SlidePawn`/`LiftPiece`/`PlacePiece` + choke points (same sites that already maintain `bbPieces`/`bbPawns`). + Replaces on-demand rebuilds in `generate.c`/`movesup.c`/`see.c`; + dedupes two byte-for-byte-identical builder functions + (`_BuildFullOccupiedBB` / `_BuildOccupiedBB`) into one. A second + position-construction path missed on the first pass + (`GenerateRandomLegalPosition` in `testsup.c`, which pokes + `rgSquare`/`bbPieces`/`bbPawns` directly, bypassing `move.c`) never + set `bbOccupied`, so the self-test suite's new consistency check + looped forever retrying a position that could never pass -- caught + because the self-test spun at 100% CPU with zero progress instead of + crashing outright, not because anything asserted immediately. +- `6e86450` -- `Eval()`'s piece-dispatch loop (`cNonPawns` walk + + `p&0x4`/`IS_KNIGHT` branch dispatch + `cDefer`/`uDefer` bookkeeping) + replaced with direct `pos->bbPieces[color][TYPE]` bitboard walks, + one type at a time, same phase order as before. Verified + byte-identical search node counts against the pre-change commit on + `tests/ecm_ringers.ep_` at `sd10` -- not just "didn't crash." +- `7e3e6b6` -- Knight: full mobility rewrite via `g_KnightAttacksBB[c]` + (already existed, built for move generation) plus bitboard masks, + replacing the per-square delta-walk and `NMobCaseTable` switch + entirely. First contributor to a new `bbMinorAttacks[2]`. +- `de3f366` -- Bishop: mobility via `_BishopAttacksBB(c, + pos->bbOccupied)` (the movegen magic-bitboard slider lookup) plus + bitboard masks, replacing the ray-walk and `BMobCaseTable`. Adds + `bbMinorXrayAttacks[2]`. **Ships a deliberate behavior change from + the old ray-walk**, not full fidelity: the old walk's + `fStop=FALSE` for x-ray-triggering blockers (friendly bishop/queen, + enemy rook/queen/king) meant it kept walking -- and kept counting + mobility -- through however many such blockers were stacked + consecutively on one ray. The bitboard version only extends one hop + past the first x-ray-worthy blocker. Found by a DEBUG assert on + `8/1R1B4/2B1r3/5k2/2P2P2/1p6/1Kb5/7n w - -` (bishop x-raying an + enemy rook, then continuing to x-ray *through* an enemy king right + behind it), and kept as-is rather than fixed with a bounded + chain-following loop -- explicit user call, for speed, given how + rare >=2 consecutive x-ray-worthy pieces on one ray is. + +### What's next, and how + +**Rook, then queen, then `_EvalKing`/`_WhoControlsSquareFast`'s final +cleanup -- one piece at a time, each landed and verified before the +next starts, not in one combined change.** This is a hard-learned +constraint, not a style preference: an earlier attempt this same +session tried to convert knight, bishop, rook, queen, and +`_EvalKing`/`_WhoControlsSquareFast` all at once after what started as +a scoping question ("why do rook/queen have to write into the old +structure too?"). It produced a real bug (a byte-scale mismatch +between the `_XRAY_BIT` whole-word constants and the standalone-byte +value `.uXray` actually reads as) that was hard to isolate with five +things changed simultaneously, and the whole thing was reverted back +to a clean commit rather than debugged further. Redone one piece at a +time afterward, each step's own bugs (the xray-scale issue again on +bishop, then the x-ray-chain gap) were caught and fixed/decided within +that single step, not entangled with four other pieces' changes. + +Concretely, for rook and then queen: + +- Mobility via `_RookAttacksBB(c, pos->bbOccupied)` / + the rook+bishop two-pass combination for queen (per section 1b, + reusing the already-tested "two passes beats one combined 8-ray + table" finding from `MOVEGEN_MIGRATION.md`), replacing + `RMobCaseTable`/`QMobCaseTable` and their ray-walks. +- New `bbRookAttacks[2]`/`bbRookXrayAttacks[2]` and + `bbQueenAttacks[2]`/`bbQueenXrayAttacks[2]`, same lifetime/clearing + discipline as the existing `bbMinorAttacks`/`bbMinorXrayAttacks` + pair. +- **Decide the x-ray-chain question for rook/queen explicitly, don't + assume bishop's answer carries over.** Bishop's single-hop + simplification was judged acceptable for bishops specifically; + rook and queen batteries (rook-behind-rook, queen-behind-rook, + queen-behind-bishop) may be common enough in real play that the + same simplification changes behavior more than bishop's did. Worth + a quick sanity check (how often do the ringers/confident/hard-quick + suites actually exercise a 2+-deep battery?) before defaulting to + the same one-hop cut, not a foregone conclusion either way. +- **The transitional helpers (`_IsSquareAttackedByMinor`, + `_IsSquareXrayedByMinor`) are explicitly temporary and get rewritten + incrementally as each piece converts, not left to accumulate special + cases.** Once rook converts, `UNSAFE_FOR_QUEEN`'s rook-bit + contribution moves from a raw `bvAttacks` read to a + `bbRookAttacks`-based check -- likely folded into + `_IsSquareAttackedByMinor`'s shape generalized to also cover rook, + or a new sibling helper, whichever reads cleaner at the time; decide + when actually doing it, not in advance. Once queen also converts, + `_EvalKing`/`_WhoControlsSquareFast`'s `bvAttack`/`bvDefend`/`bvXray` + computation stops touching `rgSquare[c|8].bvAttacks` at all, and at + that point the helpers themselves can very likely be deleted + entirely in favor of plain `pos->bbXAttacks[color] & sq` reads -- + the whole point of calling them "transitional" from the start. +- **This culminates in removing the `c|8`/`bvAttacks`/`ATTACK_BITV` + mechanism from `chess.h` entirely** (the `SQUARE` union becomes a + plain 2-field struct, already done in shape when pawn converted; + what's left is the `bvAttacks[2]` member itself and the + `ATTACK_BITV` type), once king is the only remaining writer and its + three write sites (`_EvalKing`'s two paths) move to + `bbKingAttacks[2]` the same way every other piece type did. Not a + separate "big cleanup" step -- it falls out for free once queen's + conversion lands, since nothing will be writing the old structure + anymore. +- **Keep writing real DEBUG asserts, using the still-live `rgSquare` + mailbox representation as independent ground truth, for as long as + it exists.** Both transitional helpers already do this (a from- + scratch mailbox ray-walk/table lookup, deliberately not sharing code + with the production bitboard technique being verified) -- keep this + pattern for rook and queen's own conversions too. This is *the* + reason the bishop x-ray-chain gap was caught during routine + `precommit_check.sh` smoke testing instead of surfacing as an + unexplained node-count drift days later. +- **Verification bar changes once a piece ships a deliberate + behavior change.** Byte-identical node counts (the bar for pawns, + `bbOccupied`, the dispatch loop, and knight) stop being the right + test once a step *intentionally* changes behavior, like bishop's + x-ray simplification. In that case: full `tests/ecm_ringers.ep_` at + `sd10`, checking (a) solve/no-solve parity holds exactly and (b) + node-count deltas stay in a bounded, unsurprising range (bishop's + landed at roughly -17% to +20% per position, all real, all + attributable to the one documented rule change) rather than + expecting or requiring equality. +- **After the attack-bits work is done** (rook, queen, king all + converted, `bvAttacks` deleted): revisit the rest of + `_EvalKnight`/`_EvalBishop`/`_EvalRook`/`_EvalQueen` -- the + non-mobility scoring terms this plan deliberately left alone + throughout (outpost bonuses, good/bad-bishop scoring, rook-on-open- + file, queen-out-early, etc.) -- with fresh eyes on whether each is + worth keeping as-is, rewriting, or cutting outright, not just + re-implementing them faster. Not scoped further than that yet; a + separate pass once the attack-bitboard foundation is solid under + all of them. + +### What's been learned about `Eval()` itself along the way + +- **The pawn hash is unambiguously worth keeping, measured, not + assumed.** Hit/miss cycle counts split via `EVAL_TIME` instrumentation + on one `sd12` benchmark position: hits average ~90 cycles, misses + average ~1741 cycles (~19x), 97%+ hit rate. The miss cost was never + mostly attack-bit population (that's its own ~1% bucket of total + eval cost, down from ~3.6% before pawns converted) -- it's the + isolated/doubled/duo/backward-pawn scoring loops, real per-pawn work + that recurs across sibling/ancestor search nodes often enough to be + well worth caching. Also: even a pawn-hash *hit* costs ~90 cycles, + more than "just compare a key" should cost -- the table (9MB/thread) + mostly isn't L1/L2-resident, so even a hit typically pays a real + memory round-trip. The practical takeaway: the hash's floor cost is + bounded by memory latency either way, so "is bitboard math faster + than a cache-missing read" was the wrong question to ask here -- + the read happens regardless, and the alternative (recompute always) + would cost ~19x more on every single call instead of only the ~3% + that miss. +- **`EVAL_TIME`'s per-term cycle breakdown (`root.c`'s printout, + `chess.h`'s `u64CyclesEval*` counters) is worth re-running after + each piece type converts**, not just once at the start. It already + caught two concrete, real wins directly attributable to specific + commits: `_PopulatePawnAttackBits`'s own bucket dropped from ~3.6% + to ~1.0% of total eval cost when pawns converted (same call site, + clean before/after comparison); the dispatch-loop rewrite produced + a consistent ~3-5% NPS improvement across all 11 ringers positions + at identical node counts. Cheap to re-check, concrete numbers + instead of "should be faster" intuition. +- **This whole effort should err on the side of speed, explicitly.** + Direct instruction, worth stating plainly since it's a real change + from the original plan's framing (section 0's "byte-identical + exact-score harness" bar, section 5's "must be byte-identical, any + mismatch is a real bug"): where a bitboard reduction can either (a) + preserve the old ray-walk's exact behavior at the cost of real + complexity (a bounded chain-following loop, multi-blocker + bookkeeping) or (b) ship a simpler, faster, slightly different + approximation of the same idea, prefer (b) once the behavior delta + has been measured and judged small/rare enough -- don't default to + fidelity as the tiebreaker. This is a deliberate trade against the + original plan's own stated bar, made explicitly and recorded here so + it's not mistaken for an oversight later. Section 5's exact-score + harness (never built) is now unlikely to be the right verification + tool for future steps that ship intentional behavior changes; the + ringers-suite-plus-bounded-delta check above is what's actually + being used instead. +- **The piece-dispatch loop (section 0's original target) turned out + to be a real, separate win worth landing on its own**, distinct from + any single piece type's mobility algorithm -- eliminating the + `p&0x4`/`IS_KNIGHT` branch and the `cNonPawns` mailbox re-read in + the outer loop was independently profitable (~3-5% NPS) and didn't + require any piece type's own conversion to land first, since + `pos->bbPieces[color][TYPE]` already existed and was simply unused + by `Eval()` before this. |