This is a non-functional simplification. Instead of passing the piece type
for remove_piece, we can rely on the board. The only exception is en-passant
which must be explicitly set because the destination square for the capture
is not the same as the piece to remove.
Verified also in the Chess960 castling case by running a couple of perft, see
the pull request discussion: https://github.com/official-stockfish/Stockfish/pull/2460
STC
LLR: 2.94 (-2.94,2.94) [-3.00,1.00]
Total: 18624 W: 4147 L: 4070 D: 10407
Ptnml(0-2): 223, 1933, 4945, 1938, 260
http://tests.stockfishchess.org/tests/view/5dfeaa93e70446e17e451163
No functional change
As Stockfish developers, we aim to make our code as legible and as close
to simple English as possible. However, one of the more notable exceptions
to this rule concerns operations between Squares and Bitboards.
Prior to this pull request, AND, OR, and XOR were only defined when the
Bitboard was the first operand, and the Square the second. For example,
for a Bitboard b and Square s, "b & s" would be valid but "s & b" would not.
This conflicts with natural reasoning about logical operators, both
mathematically and intuitively, which says that logical operators should
commute.
More dangerously, however, both Square and Bitboard are defined as integers
"under the hood." As a result, code like "s & b" would still compile and give
reasonable bench values. This trap occasionally ensnares even experienced
Stockfish developers, but it is especially dangerous for new developers not
aware of this peculiarity. Because there is no compilation or runtime error,
and a reasonable bench, only a close review by approvers can spot this error
when a test has been submitted--and many times, these bugs have slipped past
review. This is by far the most common logical error on Fishtest, and has
wasted uncountable STC games over the years.
However, it can be fixed by adding three non-functional lines of code. In this
patch, we define the operators when the operands are provided in the opposite
order, i.e., we make AND, OR, and XOR commutative for Bitboards and Squares.
Because these are inline methods and implemented identically, the executable
does not change at all.
This patch has the small side-effect of requiring Squares to be explicitly
cast to integers before AND, OR, or XOR with integers. This is only performed
twice in Stockfish's source code, and again does not change the executable at
all (since Square is an enum defined as an integer anyway).
For demonstration purposes, this pull request also inverts the order of one AND
and one OR, to show that neither the bench nor the executable change. (This
change can be removed before merging, if preferred.)
I hope that this pull request significantly lowers the barrier-of-entry for new
developer to join the Stockfish project. I also hope that this change will improve
our efficiency in using our generous CPU donors' machines, since it will remove
one of the most common causes of buggy tests.
Following helpful review and comments by Michael Stembera (@mstembera), we add
a further clean-up by implementing OR for two Squares, to anticipate additional
traps developers may encounter and handle them cleanly.
Closes https://github.com/official-stockfish/Stockfish/pull/2387
No functional change.
- Cleanups by Alain
- Group king attacks and king defenses
- Signature of futility_move_count()
- Use is_discovery_check_on_king()
- Simplify backward definition
- Use static asserts in move generator
- Factor a statement in move generator
No functional change
This is a non-functional simplification. Since our file_bb handles either Files or Squares, using Square here removes some code. Not likely any performance difference despite the test.
STC
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 6081 W: 1444 L: 1291 D: 3346
http://tests.stockfishchess.org/tests/view/5ceb3e2e0ebc5925cf07ab03
Non functional change.
Store repetition info in StateInfo instead of recomputing it in
three different places. This saves some work in has_game_cycle()
where this info is needed for positions before the root.
Tested for non-regression at STC:
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 34104 W: 7586 L: 7489 D: 19029
http://tests.stockfishchess.org/tests/view/5cd0676e0ebc5925cf044b56
No functional change.
We can remove the values in Pawns if we just use the piece arrays in Position. This reduces the size of a pawn entry. This simplification passed individually, and in concert with ps_passedcount100 (removes passedCount storage in pawns.).
STC
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 19957 W: 4529 L: 4404 D: 11024
http://tests.stockfishchess.org/tests/view/5cb3c2d00ebc5925cf016f0d
Combo STC
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 17368 W: 3925 L: 3795 D: 9648
http://tests.stockfishchess.org/tests/view/5cb3d3510ebc5925cf01709a
This is a non-functional simplification.
This is a non-functional code style change.
If we add an accessor function for SquareBB we can consolidate all of the asserts. This is also a bit cleaner because all SquareBB accesses go through this method making future changes easier to manage.
STC
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 63406 W: 14084 L: 14045 D: 35277
http://tests.stockfishchess.org/tests/view/5c9ea6100ebc5925cfffc9af
No functional change.
Delay legality check of castling moves at search time,
just before making the move, as is the standard with all
the other move types.
This should avoid an useless and not trivial legality check
when the castling is then not tried later. For instance due
to a previous cut-off.
The patch is also a big simplification and allows to entirely
remove generate_castling()
Bench changes due to a different move sequence out of MovePicker.
STC:
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 45073 W: 9918 L: 9843 D: 25312
http://tests.stockfishchess.org/tests/view/5c2f176f0ebc596a450bdfb3
LTC:
LLR: 3.15 (-2.94,2.94) [-3.00,1.00]
Total: 10156 W: 1707 L: 1560 D: 6889
http://tests.stockfishchess.org/tests/view/5c2e7dfd0ebc596a450bcdf4
Verified with perft both in standard and Chess960 cases.
Closes https://github.com/official-stockfish/Stockfish/pull/1929
Bench: 3559104
I've gone through the RENAME/REFORMATTING thread and changed everything I could find, plus a few more. With this, let's close the previous issue and open another.
No functional change.
Preparation commit for the upcoming Stockfish 10 version, giving a chance to catch last minute feature bugs and evaluation regression during the one-week code freeze period. Also changing the copyright dates to include 2019.
No functional change
A position which has a move which draws by repetition, or which could have
been reached from an earlier position in the game tree, is considered to be
at least a draw for the side to move.
Cycle detection algorithm by Marcel van Kervink:
https://marcelk.net/2013-04-06/paper/upcoming-rep-v2.pdf
----------------------------
How does the algorithm work in practice? The algorithm is an efficient
method to detect if the side to move has a drawing move, without doing any
move generation, thus possibly giving a cheap cutoffThe most interesting
conditions are both on line 1195:
```
if ( originalKey == (progressKey ^ stp->key)
|| progressKey == Zobrist::side)
```
This uses the position keys as a sort-of Bloom filter, to avoid the expensive
checks which follow. For "upcoming repetition" consider the opening Nf3 Nf6 Ng1.
The XOR of this position's key with the starting position gives their difference,
which can be used to look up black's repeating move (Ng8). But that look-up is
expensive, so line 1195 checks that the white pieces are on their original squares.
This is the subtlest part of the algorithm, but the basic idea in the above game
is there are 4 positions (starting position and the one after each move). An XOR
of the first pair (startpos and after Nf3) gives a key matching Nf3. An XOR of
the second pair (after Nf6 and after Ng1) gives a key matching the move Ng1. But
since the difference in each pair is the location of the white knight those keys
are "identical" (not quite because while there are 4 keys the the side to move
changed 3 times, so the keys differ by Zobrist::side). The loop containing line
1195 does this pair-wise XOR-ing.
Continuing the example, after line 1195 determines that the white pieces are
back where they started we still need to make sure the changes in the black
pieces represents a legal move. This is done by looking up the "moveKey" to
see if it corresponds to possible move, and that there are no pieces blocking
its way. There is the additional complication that, to match the behavior of
is_draw(), if the repetition is not inside the search tree then there must be
an additional repetition in the game history. Since a position can have more
than one upcoming repetition a simple count does not suffice. So there is a
search loop ending on line 1215.
On the other hand, the "no-progress' is the same thing but offset by 1 ply.
I like the concept but think it currently has minimal or negative benefit,
and I'd be happy to remove it if that would get the patch accepted. This
will not, however, save many lines of code.
-----------------------------
STC:
LLR: 2.95 (-2.94,2.94) [0.00,5.00]
Total: 36430 W: 7446 L: 7150 D: 21834
http://tests.stockfishchess.org/tests/view/5afc123f0ebc591fdf408dfc
LTC:
LLR: 2.96 (-2.94,2.94) [0.00,5.00]
Total: 12998 W: 2045 L: 1876 D: 9077
http://tests.stockfishchess.org/tests/view/5afc2c630ebc591fdf408e0c
How could we continue after the patch:
• The code in search() that checks for cycles has numerous possible variants.
Perhaps the check need could be done in qsearch() too.
• The biggest improvement would be to get "no progress" to be of actual benefit,
and it would be helpful understand why it (probably) isn't. Perhaps there is an
interaction with the transposition table or the (fantastically complex) tree
search. Perhaps this would be hard to fix, but there may be a simple oversight.
Closes https://github.com/official-stockfish/Stockfish/pull/1575
Bench: 4550412
This patch corrects both MultiPV behaviour and "go searchmoves" behaviour
for tablebases.
We change the logic of table base probing at root positions from filtering
to ranking. The ranking code is much more straightforward than the current
filtering code (this is a simplification), and also more versatile.
If the root is a TB position, each root move is probed and assigned a TB score
and a TB rank. The TB score is the Value to be displayed to the user for that
move (unless the search finds a mate score), while the TB rank determines which
moves should appear higher in a multi-pv search. In game play, the engine will
always pick a move with the highest rank.
Ranks run from -1000 to +1000:
901 to 1000 : TB win
900 : normally a TB win, in rare cases this could be a draw
1 to 899 : cursed TB wins
0 : draw
-1 to -899 : blessed TB losses
-900 : normally a TB loss, in rare cases this could be a draw
-901 to -1000 : TB loss
Normally all winning moves get rank 1000 (to let the search pick the best
among them). The exception is if there has been a first repetition. In that
case, moves are ranked strictly by DTZ so that the engine will play a move
that lowers DTZ (and therefore cannot repeat the position a second time).
Losing moves get rank -1000 unless they have relatively high DTZ, meaning
they have some drawing chances. Those get ranks towards -901 (when they
cross -900 the draw is certain).
Closes https://github.com/official-stockfish/Stockfish/pull/1467
No functional change (without tablebases).
Implements renaming suggestions by Marco Costalba, Günther Demetz,
Gontran Lemaire, Ronald de Man, Stéphane Nicolet, Alain Savard,
Joost VandeVondele, Jerry Donald Watson, Mike Whiteley, xoto10,
and I hope that I haven't forgotten anybody.
Perpetual renaming thread for suggestions:
https://github.com/official-stockfish/Stockfish/issues/1426
No functional change.
To compute dicovered check or pinned pieces we use some bitwise
operators that are not really needed because already accounted for
at the caller site.
For instance in evaluation we compute:
pos.pinned_pieces(Us) & s
Where pinned_pieces() is:
st->blockersForKing[c] & pieces(c)
So in this case the & operator with pieces(c) is useless,
given the outer '& s'.
There are many places where we can use the naked blockersForKing[]
instead of the full pinned_pieces() or discovered_check_candidates().
This path is simpler than original and gives around 1% speed up for me.
Also tested for speed by mstembera and snicolet (neutral in both cases).
No functional change.
Where variable names are explicitly incorrect, I feel morally obligated to at least
suggest an alternative. There are many, but these two are especially egregious.
No functional change.
For some reason, although game phase is used
only in material, it is computed in Position.
Move computation to material, where it belongs,
and remove the useless call chain.
No functional change.
The main change of the patch is that now time check
is done only by main thread. In the past, before lazy
SMP, we needed all the threds to check for available
time because main thread could have been blocked on
a split point, now this is no more the case and main
thread can do the job alone, greatly simplifying the logic.
Verified for regression testing on STC with 7 threads:
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 11895 W: 1741 L: 1608 D: 8546
No functional change.
Closes#1152
Now we don't need anymore the tricky pointer to
show the failed test. Added some few tests too.
Also small rename in see_ge() while there.
No functional change
Closes#1151
the nodes, tbHits, rootDepth and lastInfoTime variables are read by multiple threads, but not declared atomic, leading to data races as found by -fsanitize=thread. This patch fixes this issue. It is based on top of the CI-threading branch (PR #1129), and should fix the corresponding CI error messages.
The patch passed an STC check for no regression:
http://tests.stockfishchess.org/tests/view/5925d5590ebc59035df34b9f
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 169597 W: 29938 L: 30066 D: 109593
Whereas rootDepth and lastInfoTime are not performance critical, nodes and tbHits are. Indeed, an earlier version using relaxed atomic updates on the latter two variables failed STC testing (http://tests.stockfishchess.org/tests/view/592001700ebc59035df34924), which can be shown to be due to x86-32 (http://tests.stockfishchess.org/tests/view/592330ac0ebc59035df34a89). Indeed, the latter have no instruction to atomically update a 64bit variable. The proposed solution thus uses a variable in Position that is accessed only by one thread, which is copied every few thousand nodes to the shared variable in Thread.
No functional change.
Closes#1130Closes#1129
StepAttacks[] is misdesigned, the color dependance is specific
to pawns, and trying to generalise to king and knights, proves
neither useful nor convinient in practice.
So this patch reformats the code with the following changes:
- Use PieceType instead of Piece in attacks_() functions
- Use PseudoAttacks for KING and KNIGHT
- Rename StepAttacks[] into PawnAttacks[]
Original patch and idea from Alain Savard.
No functional change.
Closes#1086
Implement a threefold repetition detection. Below are the examples of
problems fixed by this change.
Loosing move in a drawn position.
position fen 8/k7/3p4/p2P1p2/P2P1P2/8/8/K7 w - - 0 1 moves a1a2 a7a8 a2a1
The old code suggested a loosing move "bestmove a8a7", the new code suggests "bestmove a8b7" leading to a draw.
Incorrect evaluation (happened in a real game in TCEC Season 9).
position fen 4rbkr/1q3pp1/b3pn2/7p/1pN5/1P1BBP1P/P1R2QP1/3R2K1 w - - 5 31 moves e3d4 h8h6 d4e3
The old code evaluated it as "cp 0", the new code evaluation is around "cp -50" which is adequate.
Brings 0.5-1 ELO gain. Passes [-3.00,1.00].
STC: http://tests.stockfishchess.org/tests/view/584ece040ebc5903140c5aea
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 47744 W: 8537 L: 8461 D: 30746
LTC: http://tests.stockfishchess.org/tests/view/584f134d0ebc5903140c5b37
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 36775 W: 4739 L: 4639 D: 27397
Patch has been rewritten into current form for simplification and
logic slightly changed so that return a draw score if the position
repeats once earlier but after or at the root, or repeats twice
strictly before the root. In its original form, repetition at root
was not returned as an immediate draw.
After retestimng testing both version with SPRT[-3, 1], both passed
succesfully, but this version was chosen becuase more natural. There is
an argument about MultiPV in which an extended draw at root may be sensible.
See discussion here:
https://github.com/official-stockfish/Stockfish/pull/925
For documentation, current version passed both at STC and LTC:
STC
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 51562 W: 9314 L: 9245 D: 33003
LTC
LLR: 2.96 (-2.94,2.94) [-3.00,1.00]
Total: 115663 W: 14904 L: 14906 D: 85853
bench: 5468995
In 10 of 12 calls total to Position::do_move()the givesCheck argument is
simply gives_check(m). So it's reasonable to make an overload without
this parameter, which wraps the existing version.
No functional change.
Rewrite the code in SF style, simplify and
document it.
Code is now much clear and bug free (no mem-leaks and
other small issues) and is also smaller (more than
600 lines of code removed).
All the code has been rewritten but root_probe() and
root_probe_wdl() that are completely misplaced and should
be retired altogheter. For now just leave them in the
original version.
Code is fully and deeply tested for equivalency both in
functionality and in speed with hundreds of games and
test positions and is guaranteed to be 100% equivalent
to the original.
Tested with tb_dbg branch for functional equivalency on
more than 12M positions.
stockfish.exe bench 128 1 16 syzygy.epd
Position: 2016/2016
Total 12121156 Hits 0 hit rate (%) 0
Total time (ms) : 4417851
Nodes searched : 1100151204
Nodes/second : 249024
Tested with 5,000 games match against master, 1 Thread,
128 MB Hash each, tc 40+0.4, which is almost equivalent
to LTC in Fishtest on this machine. 3-, 4- and 5-men syzygy
bases on SSD, 12-moves opening book to emphasize mid- and endgame.
Score of SF-SyzygyC++ vs SF-Master: 633 - 617 - 3750 [0.502] 5000
ELO difference: 1
No functional change.
Use a per-thread counter to reduce contention
with many cores and endgame positions.
Measured around 1% speed-up on a 12 core and 8%
on 28 cores with 6-men, searching on:
7R/1p3k2/2p2P2/3nR1P1/8/3b1P2/7K/r7 b - - 3 38
Also retire the unused set_nodes_searched() and fix
a couple of return types and naming conventions.
No functional change.
Stephane's patch removes the only usage of Position::see, where the
returned value isn't immediately compared with a value. So I replaced
this function by its optimised and more specific version see_ge. This
function also supersedes the function Position::see_sign.
bool Position::see_ge(Move m, Value v) const;
This function tests if the SEE of a move is greater or equal than a
given value. We use forward iteration on captures instread of backward
one, therefore we don't need the swapList array. Also we stop as soon
as we have enough information to obtain the result, avoiding unnecessary
calls to the min_attacker function.
Speed tests (Windows 7), 20 runs for each engine:
Test engine: mean 866648, st. dev. 5964
Base engine: mean 846751, st. dev. 22846
Speedup: 1.023
Speed test by Stephane Nicolet
Fishtest STC test:
LLR: 2.96 (-2.94,2.94) [0.00,5.00]
Total: 26040 W: 4675 L: 4442 D: 16923
http://tests.stockfishchess.org/tests/view/57f648990ebc59038170fa03
No functional change.
Don't allow pinned pieces to attack the exchange-square as long all
pinners (this includes also potential ones) are on their original
square.
As soon a pinner moves to the exchange-square or get captured on it, we
fall back to standard SEE behaviour.
This correctly handles the majority of cases with absolute pins.
bench: 6883133
