Small cleanups 05

[vondele]

Starting with an empty PR to collect suggestions for small cleanups.

No functional change.

[mstembera]

Lines 319 - 323 in movegen.cpp can be changed to

while (sliders)
      sliderAttacks |= ~sliders & LineBB[ksq][pop_lsb(&sliders)];

[mstembera]

It seems that the edge_distance() methods should return an int as our other distance methods do and is more natural.
mstembera@71c0188
Alternately, if we insist on returning File and Rank we could rename edge_distance() to something not using the word distance.

[silversolver1]

The following line

Stockfish/src/search.cpp

Line 900 in 209e942

Value raisedBeta = std::min(beta + 189 - 45 * improving, VALUE_INFINITE);

can be changed to
Value raisedBeta = beta + 189 - 45 * improving;
meaning removing the std::min comparison.
Doing so produces the same bench as current master.
This is because one of the conditions that must be true for that line to run is

Stockfish/src/search.cpp

Line 898 in 209e942

&& abs(beta) < VALUE_TB_WIN_IN_MAX_PLY)

As the following are also true (as can be seen in the types.h file),
VALUE_TB_WIN_IN_MAX_PLY = VALUE_MATE - 2 * MAX_PLY
VALUE_MATE = 32000
constexpr int MAX_PLY = 246;
VALUE_INFINITE = 32001
this means that if we assume that the absolute value of integer beta is the very highest it could be, ie, one less than VALUE_TB_WIN_IN_MAX_PLY, the maximum value that beta + 189 - 45 * improving could evaluate to is: (32000 - (2 * 246) - 1) + 189 - (45 * 0) = 31696 which will always be less than VALUE_INFINITE, which is 32001.

note that the term improving is a bool so its values can only be 0 or 1.

also note that it doesn't necessarily have to be changed as the comparison is potentially useful as a fail-safe

[xoto10]

previousScore is used in mainThread, in rootMoves and also as a local variable in search.cpp
To avoid confusion we can rename 2 of these so that the names are different, e.g. master...xoto10:prev1

[protonspring]

We can consolidate some square stepping code.

master...protonspring:ps_safesquares2

[protonspring]

Bitboard.h, line 109. We can replace the assert with is_ok(s).

[mstembera]

Lines 180 and 182 in types.h may be better as

  VALUE_TB_LOSS_IN_MAX_PLY = -VALUE_TB_WIN_IN_MAX_PLY,

  VALUE_MATED_IN_MAX_PLY   = -VALUE_MATE_IN_MAX_PLY,

especially since we now use

if (abs(bestThread->rootMoves[0].score) >= VALUE_TB_WIN_IN_MAX_PLY)

on line 293 in search.cpp.

[silversolver1]

We can move both initializing and defining bool formerPv in search.cpp higher up in the code so that that the boolean can be utilized from lines 699 onward. In its current state, it can only be used in lines 966 onwards which unnecessarily limits its usability.

As in master...silversolver1:former_pv_base. Does not change bench.

More specifically, this would allow the boolean to potentially be used from Step 4 onwards whereas currently its use is limited to Step 12 onwards. no functional change

[vondele]

Lines 319 - 323 in movegen.cpp can be changed to

while (sliders)
      sliderAttacks |= ~sliders & LineBB[ksq][pop_lsb(&sliders)];

I'm wondering about this change... the RHS changes sliders (via pop_lsb) and uses it in the same expression. For me that seems less clear, is the evaluation order even defined?

[xoto10]

We can remove the extra space near end of l754 in evaluate.cpp 22 ; to 22;

[mstembera]

@vondele I think expressions getting evaluated left to right applies here but not sure how to confirm. If I write it as

    sliderAttacks |= LineBB[ksq][pop_lsb(&sliders)] & ~sliders ;

that DOES change the bench as expected based on left to right evaluation. An alternative is also

    sliderAttacks |= LineBB[ksq][pop_lsb(&sliders)] & ~pos.checkers();

which doesn't care about order. Whatever you decide is totally fine by me.

[Vizvezdenec]

I think we should somehow label new pruning heuristic and probably SEE pruning for captures.
https://github.com/official-stockfish/Stockfish/blob/master/src/search.cpp#L1042
https://github.com/official-stockfish/Stockfish/blob/master/src/search.cpp#L1047
Smth like "capture history based pruning" and "see based pruning" will be okay I guess (?)

[protonspring]

This is a little faster on my machines and seems easier to read.

master...protonspring:ps_scores13

[MichaelB7]

@protonspring - should the comment above the code be revised?

[protonspring]

Yeah. . probably.

[protonspring]

The comment above LongDiagonalBishops (evaluate.cpp, line 319) is not clear.

It looks like the code applies LongDiagonalBishop even if there is a pawn on the further center square. I'm not sure this counts as "seeing" both center squares. Am I reading that correctly?

[Lolligerhans]

pawns.cpp:135 can use logical or ||.
Lolligerhans@4fd67d5

[Lolligerhans]

@mstembera @vondele
sliderAttacks |= LineBB[ksq][pop_lsb(&sliders)] & ~sliders ;
This is undefined behaviour as popping sliders and negating sliders are sequenced simultaneously. The same applies when exchanging the operands of &.
Call pop_lsb() separately to be explicit and readable.

E: Ah I see it doesn't matter in the end as the other Version was chosen.

[protonspring]

Bitboard.h, line 126, the second square_bb can go away since we already have a Bitboard/Square operator.

inline Bitboard operator|(Square s, Square s2) { return square_bb(s) | s2; }

[protonspring]

master...protonspring:ps_movegen9

https://tests.stockfishchess.org/tests/view/5e8f94664ff721d0bce30dc7

[protonspring]

This is not a simplification, but is a bit faster. For TrappedRook, if file_of(ksq) < FILE_E, then we can automatically assume that we can't castle because the king had to have moved. this is faster (master: 40.80, patch: 40.59). Should I test this?

master...protonspring:ps_trappedrook111

[vondele]

@protonspring if this speedup can be confirmed to be about 0.5% (is your number above, but seems a lot). it can be filed as a separate PR.

[TerjeKir]

Stockfish/src/bitboard.h

Line 212 in 195a4fe

return c == WHITE ? ~Rank1BB << 8 * (rank_of(s) - RANK_1)

This and the next line can use relative_rank().

[vondele]

@TerjeKir could you please post the diff... makes it easier for me.

[TerjeKir]

master...TerjeKir:rel_rank

[Vizvezdenec]

          // Capture history based pruning when not in check

when move doesn't give check *