could anyone review this code "search routine" ?

Discussion of chess software programming and technical issues.

Moderators: hgm, Rebel, chrisw

MahmoudUthman
Posts: 234
Joined: Sat Jan 17, 2015 11:54 pm

could anyone review this code "search routine" ?

Post by MahmoudUthman »

before I add anything else that could introduce bugs , is this code right ?
Qsearch:

Code: Select all

enum NodeType { NonPV, PV };
template <NodeType nodeType>
int qsearch&#40;int alpha, int beta&#41;
&#123;
	int bestValue, value;
	bool givesCheck;

	U64 AttackedSqs = STM ? Position&#58;&#58;AttackedSquares<Black>() &#58; Position&#58;&#58;AttackedSquares<White>();
	U64 InCheck = AttackedSqs&BitBoards&#91;STM + King&#93;;

	if &#40;InCheck&#41; bestValue = -Value_Infinite;
	else
	&#123;
		bestValue = Evaluate&#40;);
		if &#40;bestValue >= beta&#41; return bestValue;// Stand pat
		if (( nodeType == PV&#41; && bestValue > alpha&#41; alpha = bestValue;
	&#125;

	Move Available&#91;218&#93;;
	const size_t MoveCount = &#40;STM ? Position&#58;&#58;QGenerator<White>&#40;Available, InCheck, AttackedSqs&#41; &#58; Position&#58;&#58;QGenerator<Black>&#40;Available, InCheck, AttackedSqs&#41;) - Available;
	Position&#58;&#58;ScoreMoves&#40;Available, MoveCount&#41;;
	std&#58;&#58;sort&#40;Available, Available + MoveCount, Srtfun&#41;;

	for &#40;size_t i = 0; i < MoveCount; ++i&#41;
	&#123;
		if (((!InCheck&#41;&(&#40;TypeBoard&#91;Available&#91;i&#93;.To&#93; != 0&#41;/*|&#40;Available&#91;i&#93;.Type!=ENPASSANT&#41;*/)) && &#40;Position&#58;&#58;see_sign&#40;Available&#91;i&#93;) < 0&#41;) continue;

		Position&#58;&#58;MakeMove&#40;Available&#91;i&#93;);
		value = -qsearch<NT>(-beta, -alpha&#41;;
		Position&#58;&#58;UnMakeMove&#40;Available&#91;i&#93;);

		if &#40;value > bestValue&#41;
		&#123;
			bestValue = value;
			if &#40;value > alpha&#41;
			&#123;
				if (( nodeType == PV&#41; && value < beta&#41;  alpha = value;
				else  return value;// Fail high
			&#125;
		&#125;
	&#125;
	if &#40;MoveCount == 0&#41; if &#40;InCheck&#41; return -Value_Mate + ply;
	return bestValue;
&#125;
ZeroWindow:

Code: Select all

int searchNonPV&#40;int alpha, int depth&#41;/*beta=alpha+1*/
&#123;
	//basic insufficient material detection
	if &#40;IsDraw&#40;)) return Value_Draw;

	//Three-fold repetition detection&#58; try the other approaches too
	int IndexofLastIrreversible = GameRecordCounter < HalfMoveClock ? 0 &#58; GameRecordCounter - HalfMoveClock;
	for &#40;int i = GameRecordCounter - 2; i >= IndexofLastIrreversible; i--) if &#40;Positionkey == GameRecord&#91;i&#93;) return Value_Draw;

	if &#40;depth == 0&#41; return qsearch<NonPV>&#40;alpha, alpha + 1&#41;;


	int bestValue = -Value_Infinite, value;
	U64 AttackedSqs = STM ? Position&#58;&#58;AttackedSquares<Black>() &#58; Position&#58;&#58;AttackedSquares<White>();
	U64 InCheck = AttackedSqs&BitBoards&#91;STM + King&#93;;
	Move Available&#91;218&#93;;
	const size_t NMoves = &#40;STM ? Position&#58;&#58;GenerateMoves<White>&#40;Available, InCheck, AttackedSqs&#41; &#58; Position&#58;&#58;GenerateMoves<Black>&#40;Available, InCheck, AttackedSqs&#41;) - Available;

	Position&#58;&#58;ScoreMoves&#40;Available, NMoves&#41;;
	std&#58;&#58;sort&#40;Available, Available + NMoves, Srtfun&#41;;


	for &#40;size_t i = 0; i < NMoves; i++)
	&#123;
		Position&#58;&#58;MakeMove&#40;Available&#91;i&#93;);
		value = -searchNonPV&#40;-&#40;alpha + 1&#41;, depth - 1&#41;;
		Position&#58;&#58;UnMakeMove&#40;Available&#91;i&#93;);
		if &#40;value > bestValue&#41;
		&#123;
			bestValue = value;
			if &#40;value > alpha&#41;
			&#123;
				if (&#40;TypeBoard&#91;Available&#91;i&#93;.To&#93; == 0&#41;) UpdateKillersAndHistory&#40;Available&#91;i&#93;, depth&#41;;
				break;// Fail high&#58;>=beta=alpha+1
			&#125;
		&#125;
	&#125;
	if &#40;NMoves == 0&#41; bestValue = InCheck ? -Value_Mate + ply &#58; Value_Draw;
	return bestValue;
&#125;
PV:

Code: Select all

int searchPV&#40;int alpha, int beta, int depth&#41;
&#123;
	//basic insufficient material detection
	if &#40;IsDraw&#40;)) return Value_Draw;

	//Three-fold repetition detection&#58; try the other approaches too
	int IndexofLastIrreversible = GameRecordCounter < HalfMoveClock ? 0 &#58; GameRecordCounter - HalfMoveClock;
	for &#40;int i = GameRecordCounter - 2; i >= IndexofLastIrreversible; i--) if &#40;Positionkey == GameRecord&#91;i&#93;) return Value_Draw;

	if &#40;depth == 0&#41; return qsearch<PV>&#40;alpha, beta&#41;;



	int bestValue = -Value_Infinite, value;
	U64 AttackedSqs = STM ? Position&#58;&#58;AttackedSquares<Black>() &#58; Position&#58;&#58;AttackedSquares<White>();
	U64 InCheck = AttackedSqs&BitBoards&#91;STM + King&#93;;
	Move Available&#91;218&#93;;
	const size_t NMoves = &#40;STM ? Position&#58;&#58;GenerateMoves<White>&#40;Available, InCheck, AttackedSqs&#41; &#58; Position&#58;&#58;GenerateMoves<Black>&#40;Available, InCheck, AttackedSqs&#41;) - Available;

	Position&#58;&#58;ScoreMoves&#40;Available, NMoves&#41;;
	std&#58;&#58;sort&#40;Available, Available + NMoves, Srtfun&#41;;

	for &#40;size_t i = 0; i < NMoves; i++)
	&#123;
		Position&#58;&#58;MakeMove&#40;Available&#91;i&#93;);
		if &#40;i > 0&#41; value = -searchNonPV&#40;-&#40;alpha + 1&#41;, depth - 1&#41;;
		if &#40;i == 0 || &#40;value > alpha && &#40;value < beta&#41;)) value = -searchPV&#40;-beta, -alpha, depth - 1&#41;;
		Position&#58;&#58;UnMakeMove&#40;Available&#91;i&#93;);
		if &#40;value > bestValue&#41;
		&#123;
			bestValue = value;
			if &#40;value > alpha&#41;
			&#123;
				if &#40;value < beta&#41; alpha = value;
				else
				&#123;
					if (&#40;TypeBoard&#91;Available&#91;i&#93;.To&#93; == 0&#41;) UpdateKillersAndHistory&#40;Available&#91;i&#93;, depth&#41;;
					break; // Fail high
				&#125;
			&#125;
		&#125;
	&#125;
	if &#40;NMoves == 0&#41; bestValue = InCheck ? -Value_Mate + ply &#58; Value_Draw;
	return bestValue;
&#125;
RootSearch With ID:

Code: Select all

Move SearchRoot&#40;size_t maxdepth&#41;
&#123;
	Move BestMove;;
	int value;

	U64 AttackedSqs = STM ? Position&#58;&#58;AttackedSquares<Black>() &#58; Position&#58;&#58;AttackedSquares<White>();
	U64 InCheck = AttackedSqs&BitBoards&#91;STM + King&#93;;
	Move Available&#91;218&#93;;
	const size_t NMoves = &#40;STM ? Position&#58;&#58;GenerateMoves<White>&#40;Available, InCheck, AttackedSqs&#41; &#58; Position&#58;&#58;GenerateMoves<Black>&#40;Available, InCheck, AttackedSqs&#41;) - Available;

	std&#58;&#58;pair<Move, int> RootMoves&#91;218&#93;;
	Position&#58;&#58;ScoreMoves&#40;Available, NMoves&#41;;
	for &#40;size_t i = 0; i < NMoves; i++) RootMoves&#91;i&#93; = std&#58;&#58;make_pair&#40;Available&#91;i&#93;, int&#40;Available&#91;i&#93;.OrderingScore&#41;);

	std&#58;&#58;stable_sort&#40;RootMoves, RootMoves + NMoves, SrtfunRN&#41;;

	for &#40;size_t d = 1; &#40;d <= maxdepth&#41;; d++)
	&#123;
		int alpha = -Value_Infinite, beta = Value_Infinite;
		for &#40;size_t i = 0; i < NMoves; i++)
		&#123;
			Position&#58;&#58;MakeMove&#40;RootMoves&#91;i&#93;.first&#41;;
			if &#40;i > 0&#41; value = -searchNonPV&#40;-&#40;alpha + 1&#41;, d - 1&#41;;
			if &#40;i == 0 || &#40;value > alpha&#41;) value = -searchPV&#40;-beta, -alpha, d - 1&#41;;
			assert&#40;abs&#40;value&#41; < Value_Mate&#41;;
			Position&#58;&#58;UnMakeMove&#40;RootMoves&#91;i&#93;.first&#41;;

			if &#40;value > alpha&#41;
			&#123;
				alpha = value;
				BestMove=RootMoves&#91;i&#93;.first;
				RootMoves&#91;i&#93;.second = value;
			&#125;
			else RootMoves&#91;i&#93;.second = -Value_Infinite;//by what should they be ordered ?
		&#125;
		if &#40;abs&#40;alpha&#41; >= Value_Mate_In_Max_Ply&#41;
		&#123;
			auto MateIn = &#40;Value_Mate - abs&#40;alpha&#41;) / 2;
			std&#58;&#58;cout << "info depth " << d << " score mate " << &#40;alpha >= 0 ? MateIn + 1 &#58; -MateIn&#41; << " time " << TimeManager&#58;&#58;Elapsed&#40;) << " pv " << Parser&#58;&#58;MoveToAlgebraicNotation&#40;BestMove&#41; << std&#58;&#58;endl;
			break;
		&#125;
		else std&#58;&#58;cout << "info depth " << d << " score cp " << &#40;alpha * 100 / PawnValueEg&#41; << " time " << TimeManager&#58;&#58;Elapsed&#40;) << " pv " << Parser&#58;&#58;MoveToAlgebraicNotation&#40;BestMove&#41; << std&#58;&#58;endl;

		std&#58;&#58;stable_sort&#40;RootMoves, RootMoves + NMoves, SrtfunRN&#41;;
		if &#40;TimeManager&#58;&#58;Stop&#40;)) break;
	&#125;

	return BestMove;
&#125;
jdart
Posts: 4366
Joined: Fri Mar 10, 2006 5:23 am
Location: http://www.arasanchess.org

Re: could anyone review this code "search routine"

Post by jdart »

As far as I can tell it looks ok. But I think you are assuming the move generator produces only strictly legal moves.

--Jon
MahmoudUthman
Posts: 234
Joined: Sat Jan 17, 2015 11:54 pm

Re: could anyone review this code "search routine"

Post by MahmoudUthman »

jdart wrote:As far as I can tell it looks ok. But I think you are assuming the move generator produces only strictly legal moves.

--Jon
thank you , yes I use a strictly legal one.
Sven
Posts: 4052
Joined: Thu May 15, 2008 9:57 pm
Location: Berlin, Germany
Full name: Sven Schüle

Re: could anyone review this code "search routine"

Post by Sven »

Questions:

1) Does Position::QGenerator() generate all legal check evasions when InCheck is nonzero?

2) Are attack bitboards calculated from scratch in Position::AttackedSquares(), or updated incrementally in Position::MakeMove()? I would expect the former, but then I think this may be much slower than the typical IsInCheck() that starts at the king square and tries to find an attacker from all directions.

Remarks and suggestions:

3) It would look more logical to me to check the checkmate/stalemate conditions above the move loop instead of below it (although I don't think it would result in a speedup). Only a pseudo-legal move generator requires to do it below the move loop.

4) Root move ordering appears to be poor. For instance, if the best move of an iteration is tried first (which is very often the case) then all other moves get the same ordering score of -Value_Infinite. A better ordering criterion that some people are using is the number of nodes searched in the subtree of a root move. That would require to introduce node counting (which you don't seem to have according to the code you have shown).

5) TimeManager::Stop() checks for timeout but you only call it once per iteration. Calling it once every N nodes (e.g. N=4096 - to choose an arbitrary power of 2 for a faster "modulo" operation) could help to use more of the available time, since otherwise you always have to stop after an iteration if there is a realistic chance that the remaining time could be insufficient for the next iteration (so far ok since many engines do something similar). But the main problem seems to be, if an iteration takes much more time than expected you could simply lose on time since you do not check for timeout during an iteration.

6) To improve readability I suggest to rename:
- "RootMoves[...].first" and "RootMoves[...].second" into "RootMoves[...].move" and "RootMoves[...].score"
- "Available" into "MoveList"

7) The loop to detect repetition detection (where the comment says "Three-fold repetition ..." but the code actually detects a two-fold repetition which is good) could be moved into a function (maybe even into the existing IsDraw()), and it could also be rewritten like this:

Code: Select all

for &#40;int i = GameRecordCounter - 4; i >= IndexofLastIrreversible; i -= 2&#41; if &#40;Positionkey == GameRecord&#91;i&#93;) return Value_Draw;
since the current position can't be a repetition of a position in an odd-ply distance, and also not of the position two plies ago (in standard chess).

8) In qsearch() the variable "givesCheck" is unused.

9) qsearch() has "nodeType" as template argument. In contrast to that, searchNonPV() and searchPV() are two separate functions with almost identical contents. That looks inconsistent to me. I would either combine searchNonPV() and searchPV() in one template function as well, sacrificing the very tiny saving by not passing both alpha and beta as parameters to searchNonPV(), or do the opposite by splitting qsearch() into two functions (if you are excited about the extra parameter). I would prefer the former way with a template function.

10) When adding the "check extension" feature later on, it might be applicable to change the structure such that you maintain a separate search function for check evasions. That would imply a few other changes as well but I think that would further increase readability.

11) You always sort the whole move list with std::sort() before searching the first move. But in many cases you get a cutoff after the first move, leaving the remaing part of the move list unused, so sorting it was a waste. Note that the following is an optimization that should not be applied too early: you could replace the full sorting of the move list by picking the next best unused move from the move list every time you want to search a new move, swapping it to position "i" in the move list. This is considerably faster for CUT nodes but slower for ALL nodes. On average it should be faster.

12) At least according to the code you have shown you do not collect the PV. Even if it does not improve playing strength, there are certainly some benefits of adding that feature. E.g. it helps when debugging or testing the engine. And also users often like to see the PV of a chess engine.
MahmoudUthman
Posts: 234
Joined: Sat Jan 17, 2015 11:54 pm

Re: could anyone review this code "search routine"

Post by MahmoudUthman »

1-yes.
2-From scratch , I used to do it to generate completely legal king moves, but I totally forget that to replace it after adding TT Cuts & early pruning that could remove the need for move generation , I'll replace it.
(3,4,5,6,8,12) I'll replace/fix them right away, as for (4) should I only order the best move from the last iteration first and the rest by nodes, or should I order the best moves from the previous iterations first and the rest by nodes for example :

Code: Select all

#Itration      BestMove         orderAfter
1                 e2e4                e2e4...rest
2                 d2d4                d2d4,e2e4....rest
3                 d2d4                d2d4,e2e4....rest
4                 g1f3                g1f3,d2d4,e2e4....rest
7-I totally forget about the odd/even thing but why should it start from :
GameRecordCounter - 4 ? I actually add the position key to the game record in make move , is this wrong ?
9-I used different non template functions for the PV and NonPV since they should be non trivially different after adding early pruning and TT cuts "It would help readability for me", but since I haven't read about pruning in qsearch yet and don't know if there will be a big difference I used the templated version.
10-wouldn't that require 2 or 4 more functions PV,NonPV(separate or 1 templated) and Qsearch (2 separate ,or 1 templated) ?
11-I don't understand what you mean by "shouldn't be applied too early" (at high depths or too early in the engine writting)?
Sven
Posts: 4052
Joined: Thu May 15, 2008 9:57 pm
Location: Berlin, Germany
Full name: Sven Schüle

Re: could anyone review this code "search routine"

Post by Sven »

MahmoudUthman wrote:as for (4) should I only order the best move from the last iteration first and the rest by nodes, or should I order the best moves from the previous iterations first and the rest by nodes for example :

Code: Select all

#Itration      BestMove         orderAfter
1                 e2e4                e2e4...rest
2                 d2d4                d2d4,e2e4....rest
3                 d2d4                d2d4,e2e4....rest
4                 g1f3                g1f3,d2d4,e2e4....rest
I can't tell what will work best for you, this may be subject to your testing!
MahmoudUthman wrote:7-I totally forget about the odd/even thing but why should it start from :
GameRecordCounter - 4 ? I actually add the position key to the game record in make move , is this wrong ?
You go backwards in game history to find a position that might be repeated for the first time by the current position. The first position in backward direction that can be repeated is four plies backward since you need at least four moves (plies) to repeat a position. From there on, it can also be 6, 8, ... plies backward.
MahmoudUthman wrote:10-wouldn't that require 2 or 4 more functions PV,NonPV(separate or 1 templated) and Qsearch (2 separate ,or 1 templated) ?
I don't think so, only one evasion search function would be sufficient. You would always call it when the moving side is in check, even from qsearch() (here with a remaining depth of 1), and I don't see a need to have evasionPV() and evasionNonPV() functions.
MahmoudUthman wrote:11-I don't understand what you mean by "shouldn't be applied too early" (at high depths or too early in the engine writting)?
What I meant was "not too early in the development process".
MahmoudUthman
Posts: 234
Joined: Sat Jan 17, 2015 11:54 pm

Re: could anyone review this code "search routine"

Post by MahmoudUthman »

thank you , one more thing is it okay to store the score of a draw by insufficient material in the transposition table before returning a draw score

Code: Select all

alphabeta&#40;.....)
&#123;
if&#40;insufficient&#40;))
&#123;
ttStore&#40;Key,drawValue,bound&#58;&#58;exact&#41;;
return 0;
&#125;
&#125;
Ras
Posts: 2487
Joined: Tue Aug 30, 2016 8:19 pm
Full name: Rasmus Althoff

Re: could anyone review this code "search routine"

Post by Ras »

Sven Schüle wrote:11) You always sort the whole move list with std::sort() before searching the first move.
Apart from the idea with fetching the best move and putting it to the top (which is a selection-sort-as-needed):

Until you get around to that, I would at least benchmark std::sort against a Shellsort (Ciura sequence). With these short list lengths of under 100 entries, Shellsort usually is the fastest thing to do.

In case you are using GCC or CLANG, using "computed goto" can buy you additional 30% over a "clean" implementation by eliminating the outest of the three nested loops.
mar
Posts: 2555
Joined: Fri Nov 26, 2010 2:00 pm
Location: Czech Republic
Full name: Martin Sedlak

Re: could anyone review this code "search routine"

Post by mar »

Ras wrote:Until you get around to that, I would at least benchmark std::sort against a Shellsort (Ciura sequence). With these short list lengths of under 100 entries, Shellsort usually is the fastest thing to do.
std::sort usually does quicksort except for small partitions (n) where it usually falls back to insertion sort, but it may be implementation-dependent

as for switch optimization, there's a slightly more portable solution than using gcc-only extensions:

Code: Select all

switch&#40;x&#41;
&#123;
case a&#58;
case b&#58;
case c&#58;
default&#58;
    UNREACHABLE;
&#125;
where UNREACHABLE maps to

Code: Select all

__assume&#40;0&#41;
for msc
and

Code: Select all

__builtin_unreachable&#40;)
for clang/gcc, completely eliminating range checking for x (assuming all cases are dispatched via jump table and not binary search)

Not sure how it would compare to "computed goto" performance-wise, but I got a nice performance improvement for a bytecode interpreter

A scripting language (Euphoria) embedded switch case labels (computed goto) directly into byte code, eliminating opcode decoding step, one of the fastest interpreters (no assembly) I ever saw.
Ras
Posts: 2487
Joined: Tue Aug 30, 2016 8:19 pm
Full name: Rasmus Althoff

Re: could anyone review this code "search routine"

Post by Ras »

mar wrote:std::sort usually does quicksort except for small partitions (n) where it usually falls back to insertion sort, but it may be implementation-dependent
Quicksort's overhead usually sucks for such small sizes, and insertion sort is only fine for list lengths of below 10.
as for switch optimization, there's a slightly more portable solution than using gcc-only extensions:
What is needed isn't a switch-like check for discrete values, but a range check. Computed goto is the fastest way here. Switch-case always has the overhead for making sure that only the mentioned values are processed, that's rooted in the C standard.

That's also how Python got a solid 20% speedup once they changed their switch-stuff in the byte interpreter to computed goto. It's really a pity that this nice idiom still hasn't made it into the standard.