Simplify generate_castling #1923
Conversation
|
I assume the reason was to make it faster as a template parameter, but that just doesn't seem to be the case. I'll remove the pos.is_chess960 as you suggest and test performance. |
|
I removed the is_chess960() parameter and it's about 1% slower than master (2% slower than this patch), so I'd prefer to leave this patch as is. |
|
I have verified with perft on the second position of our benchmark, where there are many castling moves. For me master (1711) is still faster by 2% than this patch (1751). Maybe you need to run serveral times, first run (cold start) is not very accurate, then after 3-4 runs result stabilizes. |
|
I have pushed further and simplified out as much as I could. This is the result: --- a/src/movegen.cpp
+++ b/src/movegen.cpp
@@ -25,7 +25,7 @@
namespace {
- template<Color Us, CastlingSide Cs, bool Checks, bool Chess960>
+ template<Color Us, CastlingSide Cs, bool Checks>
ExtMove* generate_castling(const Position& pos, ExtMove* moveList) {
constexpr Color Them = (Us == WHITE ? BLACK : WHITE);
@@ -37,16 +37,14 @@ namespace {
// After castling, the rook and king final positions are the same in Chess960
// as they would be in standard chess.
+ Bitboard enemies = pos.pieces(Them);
Square kfrom = pos.square<KING>(Us);
Square rfrom = pos.castling_rook_square(Cr);
Square kto = relative_square(Us, KingSide ? SQ_G1 : SQ_C1);
- Bitboard enemies = pos.pieces(Them);
+ Direction step = kto > kfrom ? WEST : EAST;
assert(!pos.checkers());
- const Direction step = Chess960 ? kto > kfrom ? WEST : EAST
- : KingSide ? WEST : EAST;
-
for (Square s = kto; s != kfrom; s += step)
if (pos.attackers_to(s) & enemies)
return moveList;
@@ -54,7 +52,8 @@ namespace {
// Because we generate only legal castling moves we need to verify that
// when moving the castling rook we do not discover some hidden checker.
// For instance an enemy queen in SQ_A1 when castling rook is in SQ_B1.
- if (Chess960 && (attacks_bb<ROOK>(kto, pos.pieces() ^ rfrom) & pos.pieces(Them, ROOK, QUEEN)))
+ if ( pos.is_chess960()
+ && (attacks_bb<ROOK>(kto, pos.pieces() ^ rfrom) & pos.pieces(Them, ROOK, QUEEN)))
return moveList;
Move m = make<CASTLING>(kfrom, rfrom);
@@ -279,16 +278,8 @@ namespace {
if (Type != CAPTURES && Type != EVASIONS && pos.castling_rights(Us))
{
- if (pos.is_chess960())
- {
- moveList = generate_castling<Us, KING_SIDE, Checks, true>(pos, moveList);
- moveList = generate_castling<Us, QUEEN_SIDE, Checks, true>(pos, moveList);
- }
- else
- {
- moveList = generate_castling<Us, KING_SIDE, Checks, false>(pos, moveList);
- moveList = generate_castling<Us, QUEEN_SIDE, Checks, false>(pos, moveList);
- }
+ moveList = generate_castling<Us, KING_SIDE, Checks>(pos, moveList);
+ moveList = generate_castling<Us, QUEEN_SIDE, Checks>(pos, moveList);
}
return moveList;This version is still slower (1.7%) of master, but faster than yours and is a quite simplification. I have tested on full perft (all bench positions), and as expected the speed difference is almost zero (positions with castling are not so common). So I suggest to update your patch with the above code: it is a bit slower only on very specific perft tests, almost same speed in general bench perft and no speed difference in normal play....but it removes quite a bit of code. |
|
I have further simplified and opened a new PR #1924 @protonspring please check it. |
|
Closing this pull request because #1929 has been merged. Thanks to @protonspring for starting work on this subject! |
4 participants
This is non-functional.
My intuition says this should be slower, but it's about 1% faster on my machines and removes 8 lines of code. STC tests seems to indicate similar performance.
STC
LLR: 2.95 (-2.94,2.94) [-3.00,1.00]
Total: 11738 W: 2637 L: 2496 D: 6605
http://tests.stockfishchess.org/tests/view/5c2be4000ebc596a450b9318