Syntaxic sugar for Position::see_ge(move, threshold) #5529

snicolet · 2024-08-08T09:00:52Z

Syntactic sugar around Position::see_ge(), so that we can use the more readable pos.see(move) >= threshold instead of pos.see_ge(move, threshold), and similar readable code for the other comparison operators.

Patch tested locally for speed with both clang and gcc, no speed penalty found.

[Edit]
Non-regression test started here: https://tests.stockfishchess.org/tests/view/66b49f254ff211be9d4edf2a

No functional change.

github-actions · 2024-08-08T09:01:31Z

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 10300593128 / attempt 1)

peregrineshahin · 2024-08-08T09:09:16Z

src/movepick.cpp

@@ -238,7 +238,7 @@ Move MovePicker::next_move(bool skipQuiets) {
    case GOOD_CAPTURE :
        if (select<Next>([&]() {
                // Move losing capture to endBadCaptures to be tried later
-                return pos.see_ge(*cur, -cur->value / 18) ? true
+                return pos.see(*cur) >= (-cur->value / 18) ? true


Amazing PR!
Now one question I wanted to ask for so long, is that for such moves, can't we save the see value into the move struct such that we reuse it when we want to prune?, the code in search looks like redundant work to me, IIUC that should be a (nice) speedup.

No, there is no function in master (nor in this PR) to calculate the SEE value of a move, we can only compare that value to a lower bound via see_ge()

snicolet · 2024-08-08T09:10:52Z

I am convinced that this is important stuff concerning the readability and the ease of reasoning about Static Exchange Evaluation of moves, when we write or tweak our see()-related pruning algorithms in search.cpp and movepicker.cpp.

There was a furry of see() pruning tests ten years ago, and much less these days... In retrospect the introduction of pos.see_ge() in #822 , albeit nice in itself for the speed it has given, has slowed down the creativity of the team for new see() improvements, just because the syntax is more painful and we subconsciously refuse to work with it.

Compare these two versions:

    current:      if (pos.see_ge(move)) ...
                  if (!pos.see_ge(move, 1))) ...
                  if (!pos.see_ge(move, -24 * lmrDepth * lmrDepth))...

    new:          if (pos.see(move) >= 0) ...
                  if (pos.see(move) <= 0) ...
                  if (pos.see(move) < -24 * lmrDepth * lmrDepth) ...

Honestly, are we able to tell without thinking what kind of comparison we do in the first version? Now, and also in six months when we come back to the code? It takes me about 5 seconds each time I read the code in master to get each line right, and 5 more seconds to double-check!

Syntactic sugar around Position::see_ge(), so that we can use the more readable pos.see(move) >= threshold instead of pos.see_ge(move, threshold), and similar readable code for the other comparison operators. Closes official-stockfish/Stockfish#5529 No functional change.

peregrineshahin · 2024-08-08T09:20:48Z

src/search.cpp

@@ -1019,7 +1019,7 @@ Value Search::Worker::search(
                lmrDepth = std::max(lmrDepth, 0);

                // Prune moves with negative SEE (~4 Elo)
-                if (!pos.see_ge(move, -24 * lmrDepth * lmrDepth))
+                if (pos.see(move) < -24 * lmrDepth * lmrDepth)


No, there is no function in master (nor in this PR) to calculate the SEE value of a move, we can only compare that value to an upper bound or a lower bound via see_ge()

@snicolet I'm referring to here, aren't we calling the function again on the move, if we are passing move to the function aren't we calculating the result for such move in this position.. is this call not redundant to the ones in the movepicker?

if we save the result S. and reuse it?

this is not a function, this is just syntaxic sugar...

pos.see(move) < A

is strictly equivalent to

!pos.see_ge(move, A)

Ok, I see so it's like alpha-beta?
So you are saying that the value is not exact for some reason,
is it not possible to match bounds at least, just something like the saved bounds in TT, where we at least match windows/thresholds?
&& (ttData.bound & (ttData.value >= beta ? BOUND_LOWER : BOUND_UPPER)))

i.e. if a see_ge value fails high over +3000, is calculating see_ge of threshold -90000 relevant?

Syntactic sugar around Position::see_ge(), so that we can use the more readable pos.see(move) >= threshold instead of pos.see_ge(move, threshold), and similar readable code for the other comparison operators. Closes official-stockfish/Stockfish#5529 No functional change.

cj5716 · 2024-08-08T09:46:01Z

I think a non-reg STC would be good, because this has shown to be quite the slowdown for many engines

Edit: sorry, I misread and missed the operator overloading

Great stuff in this case

Disservin

good idea imo, please use using instead of typedef to be consistent with the code

what i don't like is that there are now two ways of doing this and not one consistent one, i think it makes sense if the see_ge function is private and only useable over the see function, but then i also don't like that the return type of see() (the pair) is useable

something like this should cover both cases and prevent any misuse
https://godbolt.org/z/nve76Kh5x

snicolet · 2024-08-08T09:54:48Z

I have tested speed locally with both clang and gcc, and found no speed penalty.

two runs of bench at depth 22 in parallel (clang 11):

testedpatch
Total time (ms) : 59078
Nodes searched  : 38781020
Nodes/second    : 656437

master
Total time (ms) : 59239
Nodes searched  : 38781020
Nodes/second    : 654653

two runs of bench at depth 22 in parallel (gcc 14)

testedpatch
Total time (ms) : 55748
Nodes searched  : 38781020
Nodes/second    : 695648

master
Total time (ms) : 55947
Nodes searched  : 38781020
Nodes/second    : 693174

Syntactic sugar around Position::see_ge(), so that we can use the more readable pos.see(move) >= threshold instead of pos.see_ge(move, threshold), and similar readable code for the other comparison operators. Closes official-stockfish/Stockfish#5529 No functional change.

Syntactic sugar around Position::see_ge(), so that we can use the more readable pos.see(move) >= threshold instead of pos.see_ge(move, threshold), and similar readable code for the other comparison operators. Patch tested locally for speed with both clang and gcc, no speed penalty found. Non-regression test started here: https://tests.stockfishchess.org/tests/view/66b49f254ff211be9d4edf2a Closes official-stockfish#5529 No functional change.

peregrineshahin · 2024-08-08T11:59:29Z

Sorry about the noise I know this PR is only related to syntaxtic sugar but I was wondering about the bigger picture,
here is a practical proof of what I claim to be redundent work:
master...peregrineshahin:Stockfish:seeStuff1
We indeed have redundent logic.

// SEE based pruning for captures and checks (~11 Elo)
int  seeHist          = std::clamp(captHist / 32, -182 * depth, 166 * depth);
auto extMove          = mp.current();
int  captThreshold    = -168 * depth - seeHist;
int  extMoveThreshold = -extMove->value / 18;

if (extMove && ((mp.stage == GOOD_CAPTURE && extMoveThreshold > captThreshold)))
    dbg_hit_on(!pos.see_ge(move, captThreshold), 0);

if (extMove && ((mp.stage == BAD_CAPTURE && extMoveThreshold < captThreshold)))
    dbg_hit_on(pos.see_ge(move, captThreshold), 1);

// Hit #0: Total 16445 Hits 0 Hit Rate (%) 0
// Hit #1: Total 14913 Hits 0 Hit Rate (%) 0

// ===========================
// Total time (ms) : 3730
// Nodes searched  : 1277466

if (!pos.see_ge(move, -168 * depth - seeHist))
    continue;

cj5716 · 2024-08-08T12:05:48Z

There is no guarantee where a good capture will beat the SEE threshold (and vice versa), though, because the way we determine good captures in movepicker is not a fixed value but relies on capture history

peregrineshahin · 2024-08-08T12:09:51Z

Yes, so what you do is this to skip the work.......
You can access the movepicker score in search as I demonstarted

if (mp.stage == GOOD_CAPTURE && extMoveThreshold >= captThreshold)
{}
else if ((mp.stage == BAD_CAPTURE && extMoveThreshold <= captThreshold) || !pos.see_ge(move, captThreshold))
    continue;

cj5716 · 2024-08-08T12:17:52Z

ah, I see what you mean. that could be a possible speedup

xu-shawn · 2024-08-10T08:33:19Z

The non-regression test had passed:
LLR: 2.95 (-2.94,2.94) <-1.75,0.25>
Total: 110144 W: 28527 L: 28388 D: 53229
Ptnml(0-2): 370, 12568, 29086, 12649, 399

https://tests.stockfishchess.org/tests/view/66b293024ff211be9d4edd7e

Disservin · 2024-08-10T09:30:07Z

just wondering would it make sense if we move see out of position? after all it is an evaluation (static exchange evaluation)

see https://github.com/official-stockfish/Stockfish/compare/master...Disservin:Stockfish:see-sugar-smth?expand=1

peregrineshahin reviewed Aug 8, 2024

View reviewed changes

snicolet force-pushed the see_sugar3 branch from 99bfe5d to 29d1260 Compare August 8, 2024 09:17

peregrineshahin reviewed Aug 8, 2024

View reviewed changes

snicolet force-pushed the see_sugar3 branch from 29d1260 to d0e26e1 Compare August 8, 2024 09:40

Disservin reviewed Aug 8, 2024

View reviewed changes

snicolet force-pushed the see_sugar3 branch from d0e26e1 to f5880fe Compare August 8, 2024 10:25

snicolet force-pushed the see_sugar3 branch from f5880fe to f248ab1 Compare August 8, 2024 10:41

Disservin added feature/functionality no-functional-change labels Aug 11, 2024

ghost mentioned this pull request Sep 6, 2024

Optimisation of Position::see and Position::see_sign atumanian/Stockfish#12

Merged

ruidazeng approved these changes Nov 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Syntaxic sugar for Position::see_ge(move, threshold) #5529

Syntaxic sugar for Position::see_ge(move, threshold) #5529

snicolet commented Aug 8, 2024 •

edited

Loading

github-actions bot commented Aug 8, 2024 •

edited

Loading

peregrineshahin Aug 8, 2024

snicolet Aug 8, 2024 •

edited

Loading

snicolet commented Aug 8, 2024 •

edited

Loading

peregrineshahin Aug 8, 2024

peregrineshahin Aug 8, 2024

snicolet Aug 8, 2024

peregrineshahin Aug 8, 2024

peregrineshahin Aug 8, 2024 •

edited

Loading

cj5716 commented Aug 8, 2024 •

edited

Loading

Disservin left a comment •

edited

Loading

snicolet commented Aug 8, 2024 •

edited

Loading

peregrineshahin commented Aug 8, 2024

cj5716 commented Aug 8, 2024

peregrineshahin commented Aug 8, 2024 •

edited

Loading

cj5716 commented Aug 8, 2024

xu-shawn commented Aug 10, 2024

Disservin commented Aug 10, 2024

Syntaxic sugar for Position::see_ge(move, threshold) #5529

Are you sure you want to change the base?

Syntaxic sugar for Position::see_ge(move, threshold) #5529

Conversation

snicolet commented Aug 8, 2024 • edited Loading

github-actions bot commented Aug 8, 2024 • edited Loading

peregrineshahin Aug 8, 2024

Choose a reason for hiding this comment

snicolet Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

snicolet commented Aug 8, 2024 • edited Loading

peregrineshahin Aug 8, 2024

Choose a reason for hiding this comment

peregrineshahin Aug 8, 2024

Choose a reason for hiding this comment

snicolet Aug 8, 2024

Choose a reason for hiding this comment

peregrineshahin Aug 8, 2024

Choose a reason for hiding this comment

peregrineshahin Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

cj5716 commented Aug 8, 2024 • edited Loading

Disservin left a comment • edited Loading

Choose a reason for hiding this comment

snicolet commented Aug 8, 2024 • edited Loading

peregrineshahin commented Aug 8, 2024

cj5716 commented Aug 8, 2024

peregrineshahin commented Aug 8, 2024 • edited Loading

cj5716 commented Aug 8, 2024

xu-shawn commented Aug 10, 2024

Disservin commented Aug 10, 2024

snicolet commented Aug 8, 2024 •

edited

Loading

github-actions bot commented Aug 8, 2024 •

edited

Loading

snicolet Aug 8, 2024 •

edited

Loading

snicolet commented Aug 8, 2024 •

edited

Loading

peregrineshahin Aug 8, 2024 •

edited

Loading

cj5716 commented Aug 8, 2024 •

edited

Loading

Disservin left a comment •

edited

Loading

snicolet commented Aug 8, 2024 •

edited

Loading

peregrineshahin commented Aug 8, 2024 •

edited

Loading