Remove redundant TT miss data reassignments

[FauziAkram]

Currently, in both the main search() and qsearch(), we manually guard ttData.move and ttData.value against ttHit.

However, looking at the TT implementation, if tt.probe() misses, it is guaranteed to return a TTData object already safely initialized with Move::none() and VALUE_NONE. Furthermore, the value_from_tt() helper natively handles VALUE_NONE by immediately passing it back out.

Because of these internal guarantees, I think we can safely drop them in both search functions?!

I'm not very expert in this part of the code, if there is anything wrong with my logic, or any drawback that I didn't think about, please just let me know.

New Update:
Passed non-reg STC test:
LLR: 5.07 (-2.94,2.94) <-1.75,0.25>
Total: 645408 W: 164134 L: 164382 D: 316892
Ptnml(0-2): 1574, 69580, 180635, 69350, 1565
https://tests.stockfishchess.org/tests/view/6a10d1c6818cacc1db0ac08e

No functional change

[github-actions]

(execution 26132247309 / attempt 1)

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 97bc0191-e6c2-458c-b7b2-ae7c50cd5482

📥 Commits

Reviewing files that changed from the base of the PR and between 0c5b04e and 995fd54.

📒 Files selected for processing (2)

• src/search.cpp
• src/tt.cpp

---

📝 Walkthrough

Walkthrough

This PR refactors TT probe handling: TranspositionTable::probe() now returns explicit empty TTData for matching-but-unoccupied entries. search() and qsearch() assert TT-miss invariants, always convert probed values with value_from_tt(), and preserve TT-provided moves except when root logic overwrites the move.

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[mstembera]

I have no strong opinions on this as I rarely work on search but just for some context
#5503
#5766 (comment)

[anematode]

Seems fine, an assert wouldn't hurt

[github-actions]

(execution 26257502910 / attempt 1)

[FauziAkram]

Added a couple of asserts as suggested by anematode, to make sure we catch future issues.

[github-actions]

(execution 26257530496 / attempt 1)

[anematode]

Oh I see.

std::tuple<bool, TTData, TTWriter> TranspositionTable::probe(const Key key) const {

    TTEntry* const tte   = first_entry(key);
    const uint16_t key16 = uint16_t(key);  // Use the low 16 bits as key inside the cluster

    for (int i = 0; i < ClusterSize; ++i)
        if (tte[i].key16 == key16)
            // This gap is the main place for read races.
            // After `read()` completes that copy is final, but may be self-inconsistent.
            return {tte[i].is_occupied(), tte[i].read(), TTWriter(&tte[i])}; // <-- 💩 
     // ...
}

If our 16-bit subkey is 0, which happens with probability 1/65536, even if the TT entry isn't occupied it will still match. Thus it'll return { false, Move::none(), VALUE_ZERO } rather than VALUE_NONE.

So I think we can't make this change without a corresponding change to tt.cpp, for example:

std::tuple<bool, TTData, TTWriter> TranspositionTable::probe(const Key key) const {

    TTEntry* const tte   = first_entry(key);
    TTEntry* replace = tte;
    const uint16_t key16 = uint16_t(key);  // Use the low 16 bits as key inside the cluster

    for (int i = 0; i < ClusterSize; ++i)
        if (tte[i].key16 == key16) {
            if (!tte[i].is_occupied()) { replace = &tte[i]; goto no_hit; }
            
            // This gap is the main place for read races.
            // After `read()` completes that copy is final, but may be self-inconsistent.
            return {true, tte[i].read(), TTWriter(&tte[i])};
        }   

    // Find an entry to be replaced according to the replacement strategy
    for (int i = 1; i < ClusterSize; ++i)
        if (replace->depth8 - 8 * replace->relative_age(generation8)
            > tte[i].depth8 - 8 * tte[i].relative_age(generation8))
            replace = &tte[i];

no_hit:
    return {false, TTData{Move::none(), VALUE_NONE, VALUE_NONE, DEPTH_NONE, BOUND_NONE, false},
            TTWriter(replace)};
}

[github-actions]

clang-format 20 needs to be run on this PR.
If you do not have clang-format installed, the maintainer will run it when merging.
For the exact version please see https://packages.ubuntu.com/plucky/clang-format-20.

(execution 26285146405 / attempt 1)

[FauziAkram]

I've just pushed an update to include the fix for tt.cpp as discussed.

It should now properly handle the edge case where an empty entry artificially matches a 0 subkey. If it matches but isn't occupied, it immediately returns the VALUE_NONE defaults and uses that empty slot for the writer (implemented directly without needing a goto).

Thanks to anematode for catching that 1/65536 edge case. Let me know if everything looks good to go now.

[github-actions]

clang-format 20 needs to be run on this PR.
If you do not have clang-format installed, the maintainer will run it when merging.
For the exact version please see https://packages.ubuntu.com/plucky/clang-format-20.

(execution 26285164804 / attempt 1)

[anematode]

Result of 100 runs
==================
base (...kfish.master) =    1930618  +/- 4204
test (./stockfish    ) =    1939604  +/- 3369
diff                   =      +8986  +/- 5260

speedup         = +0.0047
P(speedup > 0) =  0.9996

I get a consistent speedup. Probably worth a non-regression fishtest tho

[FauziAkram]

Passed non-reg STC test:
https://tests.stockfishchess.org/tests/view/6a10d1c6818cacc1db0ac08e