Avoid unnecessarily initializing TTData with 0's.

[mstembera]

The compiler generated default constructor is initializing TTData with 0's when we invoke it here

Stockfish/src/tt.cpp

Line 241 in c76c179

return {false, TTData(), TTWriter(replace)};

An empty default constructor avoids this as shown here https://godbolt.org/z/a8q8o3EEz
(For some reason gcc initializes to 0 even with an empty constructor which seems like a gcc bug?)

No functional change
bench: 999324

[github-actions]

clang-format 18 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/noble/clang-format-18.

(execution 12732451394 / attempt 1)

[mstembera]

I think this exposes that we are making use of ttData somewhere in search without checking that ttHit is true. I suspect this is why the PR checks are failing. Issue #5503

[Disservin]

your godbolt link is with clang? and if I use gcc it still zeros it even with your change?
https://godbolt.org/z/sMjWsnf14 nvm

why did you add this ?
#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" I don't get any warning here

[Disservin]

Okay so for the sanitizer warning I've opened a bug report https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118436.

The gcc behaviour is weird.. though I mean this is such a micro optimization patch and the only thing it highlights is maybe something wrong in search but i'd really prefer stuff be properly initialized unless it's causing a real measurable performance degradation

[Disservin]

i opened a bug report for the gcc initialization case (i hope that was fine with you) https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118440, lets see what they say..

update: duplicate of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61810

[Disservin]

to fix your branch you'd need to add those...

diff --git a/src/search.cpp b/src/search.cpp
index e8a9f0b9..49805c32 100644
--- a/src/search.cpp
+++ b/src/search.cpp
@@ -853,7 +853,7 @@ Value Search::Worker::search(
     // If we have a good enough capture (or queen promotion) and a reduced search
     // returns a value much above beta, we can (almost) safely prune the previous move.
     probCutBeta = beta + 187 - 56 * improving;
-    if (!PvNode && depth > 3
+    if (!PvNode && depth > 3 && ttHit
         && !is_decisive(beta)
         // If value from transposition table is lower than probCutBeta, don't attempt
         // probCut there and in further interactions with transposition table cutoff
@@ -922,8 +922,9 @@ moves_loop:  // When in check, search starts here

     // Step 12. A small Probcut idea (~4 Elo)
     probCutBeta = beta + 417;
-    if (ttHit && (ttData.bound & BOUND_LOWER) && ttData.depth >= depth - 4 && ttData.value >= probCutBeta
-        && !is_decisive(beta) && is_valid(ttData.value) && !is_decisive(ttData.value))
+    if (ttHit && (ttData.bound & BOUND_LOWER) && ttData.depth >= depth - 4
+        && ttData.value >= probCutBeta && !is_decisive(beta) && is_valid(ttData.value)
+        && !is_decisive(ttData.value))
         return probCutBeta;

     const PieceToHistory* contHist[] = {(ss - 1)->continuationHistory,
@@ -1150,7 +1151,8 @@ moves_loop:  // When in check, search starts here

         // Decrease reduction if position is or has been on the PV (~7 Elo)
         if (ss->ttPv)
-            r -= 1024 + (ttData.value > alpha) * 1024 + (ttData.depth >= depth) * 1024;
+            r -= 1024 + (ttHit && ttData.value > alpha) * 1024
+               + (ttHit && ttData.depth >= depth) * 1024;

         // Decrease reduction for PvNodes (~0 Elo on STC, ~2 Elo on LTC)
         if (PvNode)
@@ -1164,7 +1166,7 @@ moves_loop:  // When in check, search starts here

         // Increase reduction for cut nodes (~4 Elo)
         if (cutNode)
-            r += 2518 - (ttData.depth >= depth && ss->ttPv) * 991;
+            r += 2518 - (ttHit && ttData.depth >= depth && ss->ttPv) * 991;

         // Increase reduction if ttMove is a capture but the current move is not a capture (~3 Elo)
         if (ttCapture && !capture)
@@ -1175,7 +1177,7 @@ moves_loop:  // When in check, search starts here
             r += 938 + allNode * 960;

         // For first picked move (ttMove) reduce reduction (~3 Elo)
-        else if (move == ttData.move)
+        else if (ttHit && move == ttData.move)
             r -= 1879;

         if (capture)
@@ -1227,7 +1229,7 @@ moves_loop:  // When in check, search starts here
         else if (!PvNode || moveCount > 1)
         {
             // Increase reduction if ttMove is not present (~6 Elo)
-            if (!ttData.move)
+            if (ttHit && !ttData.move)
                 r += 2037;

             // Note that if expected reduction is high, we reduce search depth by 1 here (~9 Elo)
@@ -1527,7 +1529,7 @@ Value Search::Worker::qsearch(Position& pos, Stack* ss, Value alpha, Value beta)
     pvHit        = ttHit && ttData.is_pv;

     // At non-PV nodes we check for an early TT cutoff
-    if (!PvNode && ttData.depth >= DEPTH_QS
+    if (!PvNode && ttHit && ttData.depth >= DEPTH_QS
         && is_valid(ttData.value)  // Can happen when !ttHit or when access race in probe()
         && (ttData.bound & (ttData.value >= beta ? BOUND_LOWER : BOUND_UPPER)))
         return ttData.value;

this patch makes the code generally too prone errors, let there be default initialization and maybe change it to good default, i.e. VALUE_NONE

[mstembera]

@Disservin
Thanks for opening the bug reports.
I tried making the changes to search you proposed above but some of them change bench even in master and the github compile checks still don't pass either way. See #5771 My understanding of search is really poor but I think our current situation is as follows:

1. Currently we unwittingly rely in search on the compiler initialization of TTData to 0's when ttHit is false.
2. Initializing to 0's is wrong and further it's questionable whether the compiler should be doing it at all.
   This seems like a fragile search implementation. As an example from your suggested fixes above changing line 1223 (in latest master) from if (!ttData.move) to if (ttHit && !ttData.move) changes bench from 1379150 to 1425435. The question is what did the line intend in the first place? i.e., were the cases of not having a tt move due to no ttHit versus having a ttHit but without a move supposed to be handled the same or differently? This isn't clear because the original author never checked for ttHit. Hopefully someone who understands search may clean these cases up.
   I have submitted https://tests.stockfishchess.org/tests/view/67845f51460e2910c51ddd07 as suggested so that for now we at least initialize to correct values since the initialization is happening anyway. On the other hand continuing to initialize when ttHit is false will perpetuate sloppy handling of ttData in search in the future.

[xu-shawn]

changing line 1223 (in latest master) from if (!ttData.move) to if (ttHit && !ttData.move) changes bench from 1379150 to 1425435. The question is what did the line intend in the first place?

A TT entry doesn't have to have a TT move, so the intended condition is probably !ttHit || !ttData.move

[Disservin]

The zeroing of the struct is normal under cpp rules afaik. The only thing gcc doesn’t do right is, that it apparently keeps this zeroing even with a user defined constructor.

Yeah id much rather have that patch pass than others, though I’m not sure if that depth entry offset makes sense, 0 is probably less weird.

Checking for tthit at every place we use tt data slows it down more than zeroing/initialing it to sane defaults I imagine.

I didn’t quite check my suggested changes in great detail just made sure valgrind was happy locally. (Use clang to compile and then run valgrind)

having to use tthit everywhere is too brittle I fear

[mstembera]

Agree. I used DEPTH_ENTRY_OFFSET instead of 0 to make sure it's lower than DEPTH_UNSEARCHED. Do you want me to stop the test and resubmit with 0?

[mstembera]

Actually I forgot it already passed https://tests.stockfishchess.org/tests/view/6757757686d5ee47d9541de9 when I was trying to fix #5503 Is that acceptable?

[xu-shawn]

Agree. I used DEPTH_ENTRY_OFFSET instead of 0 to make sure it's lower than DEPTH_UNSEARCHED. Do you want me to stop the test and resubmit with 0?

0 might conflict with the value of DEPTH_QS, maybe just use DEPTH_UNSEARCHED for this?

[Disservin]

makes more sense from a naming standpoint, I think the depth value isn't really used anywhere relevant so -3 and -2 should have no diff?

[Disservin]

but yeah that patch @mstembera generally looks fine

[mstembera]

In practice with the current code base there is no diff between -3 or -2. However, DEPTH_UNSEARCHED is still a value that gets written to the TT by search and is considered a valid depth.

Stockfish/src/search.cpp

Line 753 in c085670

ttWriter.write(posKey, VALUE_NONE, ss->ttPv, BOUND_NONE, DEPTH_UNSEARCHED, Move::none(),

DEPTH_ENTRY_OFFSET on the other hand is what you would get from reading a TT entry that has never been written to. It is and invalid depth lower than anything else and so probably the safest thing to return regardless of any future changes. Maybe we could change the name of DEPTH_ENTRY_OFFSET to something better like DEPTH_NONE or DEPTH_INVALID or something else?