Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small cleanups 04 #2584

Closed
wants to merge 13 commits into from
Closed

Conversation

vondele
Copy link
Member

@vondele vondele commented Mar 14, 2020

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

No functional change.

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

No functional change.
@mstembera
Copy link
Contributor

Line 322 in movegen.cpp should change from

sliderAttacks |= LineBB[checksq][ksq] ^ checksq;

to

sliderAttacks |= LineBB[ksq][checksq] ^ checksq;

because ksq doesn't change but checksq does so it's more efficient.

@Vizvezdenec
Copy link
Contributor

https://github.com/official-stockfish/Stockfish/blob/master/src/evaluate.cpp#L316
this bool is not needed since we use !(bitboard) - it's automatically converted into bool.

@Vizvezdenec Vizvezdenec mentioned this pull request Mar 18, 2020
@protonspring
Copy link

We can compact some initialization in bitboard.cpp.

master...protonspring:ps_bitboard2

@Rocky640
Copy link

Rocky640 commented Mar 20, 2020

evaluate.cpp lines94-102, remove the "s" to Knight, Bishop, Rook, Queen

rewrite comment
// Knight and Bishop bonus for being right behind a pawn
to
// Bonus for knight and bishop shielded by a pawn

position.cpp line 669
Square rfrom = to; // Castling is encoded as 'king captures the rook'

(no capital letters for chess pieces in general in comments)

@protonspring
Copy link

There is an extra space after depth in movepick.cpp, Line 62.

@mstembera
Copy link
Contributor

Very minor nit pick but similar to the LineBB[][] change line 306 in evaluate.cpp
could go from
score -= KingProtector * distance(s, pos.square(Us));
to
score -= KingProtector * distance(pos.square(Us), s);
because the king square is staying the same while s is changing and distance()
is accessing SquareDistance[][] under the hood. In practice there is no difference.

@protonspring
Copy link

It looks like the Cuckoo tables go through all of the pieces and use PseudoAttacks[pt].
Of course, PseudoAttacks for pawns is not valid and is always 0, so this is confusing.

It might be better for position.cpp line 139 to say: for (Piece pc : {ROOK, KNIGHT, BISHOP, QUEEN, KING}) or similar.

@TerjeKir
Copy link

https://github.com/official-stockfish/Stockfish/blob/master/src/search.cpp#L870
Comment should reflect that this prevents not only mate scores, but also TB win scores.

@vondele
Copy link
Member Author

vondele commented Mar 24, 2020

@protonspring that would require a longer list (Piece vs PieceType), so don't think this is worthwhile?

@protonspring
Copy link

I agree. . perhaps just a comment that pawns are ignored in the cuckoo tables.

@vondele
Copy link
Member Author

vondele commented Mar 24, 2020

yeah, that comment is somewhat hidden (at the declaration of the arrays, only reversible moves)

@mstembera
Copy link
Contributor

I just realized that line 322 in movegen.cpp which I commented about above doesn't need to be xoring each checksq inside the loop one at a time. Instead the whole block

  while (sliders)
  {
      Square checksq = pop_lsb(&sliders);
      sliderAttacks |= LineBB[checksq][ksq] ^ checksq;
  }

can be simplified to

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

removing all the checkers at once at the end. I verified bench is the same up to depth 28 774011575.

@Rocky640
Copy link

Since (sliders) is empty most of the time, I believe that on average
the change you propose will end up with more operations. Can you run some statistics on a bench ?

@protonspring
Copy link

@mstembera Your change to movegen.cpp line 322 is a little slower on my machines.
100 runs of bench ave:
master = 3.032
Patch = 3.034

@mstembera
Copy link
Contributor

@Rocky640 You are right. On average the code inside of the while loop executes only 80% of the time generate() is called. Do you think the following is a better way to simplify?

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

or

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

@protonspring Thanks. I can't measure any real speed difference between any of the versions.

@mstembera
Copy link
Contributor

Lines 258 and 259 in bitboard.h edge_distance(File f) and edge_distance(Rank r) return File and Rank respectively. Would it make more sense to have them return int or name them better w/o using the word distance? All of our other distance methods return int.

@protonspring
Copy link

protonspring commented Mar 29, 2020

@mstembera Then, all of the calls would have be edge_distance(file_of(x)), which might be kinda ugly ?

EDIT: After looking at it, it's not that bad. How about?
https://tests.stockfishchess.org/tests/view/5e802e7ee42a5c3b3ca2ed17

@vondele vondele mentioned this pull request Mar 29, 2020
@mstembera
Copy link
Contributor

@protonspring I don't see any reason to add file_of(x) when passing parameters to edge_distance().

@Lolligerhans
Copy link
Contributor

Lolligerhans commented Mar 30, 2020

Just wanna nudge a decision on possible specialization for Position:count<KING>, which can return 2 for no parameters and 1 in the overload with a Color parameter.

Are these cases required for anything (testing, tuning, invalid positions)? Otherwise it may (?) be a good idea to specialize, remove or delete them.

@vondele
Copy link
Member Author

vondele commented Mar 30, 2020

@Lolligerhans you refer to this code, I assume, but I'm not quite sure what your observation is:

Stockfish/src/position.h

Lines 246 to 252 in f2430bf

template<PieceType Pt> inline int Position::count(Color c) const {
return pieceCount[make_piece(c, Pt)];
}
template<PieceType Pt> inline int Position::count() const {
return pieceCount[make_piece(WHITE, Pt)] + pieceCount[make_piece(BLACK, Pt)];
}

@Lolligerhans
Copy link
Contributor

Lolligerhans commented Mar 30, 2020

@vondele I found myself use the member Position::count<KING> in a weak moment. Since master does not use it anywhere I think it would logical to have either

template<> inline int Position::count<KING>(Color c) const { 
   (void) c;
   return 1;
} 
  
 template<> inline int Position::count<KING>() const { 
   return 2;
}

to obtain best speed or

template<> inline int Position::count<KING>(Color c) const = delete;
template<> inline int Position::count<KING>() const = delete;

to prevent usage or to remove the instantiation entirely.

I think it makes sense to specialize, allowing pos.count<ALL_PIECES>() - pos.count<KING>(), for example.

@vondele
Copy link
Member Author

vondele commented Mar 30, 2020

I see ... <KING> was missing (eaten by markdown, I assume) in your previous post... making it a bit harder to understand ;-). For more choice, one could also add an assert to the existing routines... not sure what the best choice is here.

@Rocky640
Copy link

@Lolligerhans

pieceCount[W_KING] is well defined, and is 1
pieceCount[B_KING] is well defined, and is 1
For example, in pos_is_ok(), we test for this (in debug mode)

I cannot test at this moment but I believe pos.count<KING>() is well defined too ?

So what is the problem ?

Specializing? Definitively not, If you want to speedup pos.count<ALL_PIECES>() - pos.count<KING>() than just go for pos.count<ALL_PIECES>() - 2. This is trivial.

Remove/Delete? It would make things uglier, and less consistent...
Maybe exe will be a bit smaller, unless you can prove some speed up, which I doubt.

@protonspring
Copy link

@Rocky640
Copy link

@protonspring

I prefer master on this one

In many place we use things such as this, for readability
a = b | c;
a &= d;

and we let the compiler optimize all this

@Lolligerhans
Copy link
Contributor

@vondele Ah yes, the <KING> got eaten. Sry I did not notice it.

@Rocky640 I do believe pos.count<ALL_PIECES>() - 2 to induce more ugliness than pos.count<ALL_PIECES>() - pos.count<KING>(). The executable remains unaffected in either case, since the member Position::count currently remains unused - presumably due to the lack of a suitable implementation.

vondele added a commit to vondele/Stockfish that referenced this pull request Mar 30, 2020
@vondele vondele closed this Mar 30, 2020
@vondele
Copy link
Member Author

vondele commented Mar 30, 2020

merged

@Rocky640
Copy link

@mstembera

sorry for late reply
the following seems like a good simplification to me.

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

@mstembera
Copy link
Contributor

@Rocky640 No problem Thanks! I already submitted the other version here
#2606
because it uses less total variables. However if you prefer this one please comment there. My preference isn't very strong either way.

@mstembera mstembera mentioned this pull request Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants