Keep low ply history from previous move

[xoto10]

This patch keeps the low-ply history from the previous move, shifting the data down by 2 ply.

Tested with closedpos book:

STC:
LLR: 2.93 (-2.94,2.94) {-0.50,1.50}
Total: 71584 W: 14175 L: 13891 D: 43518
Ptnml(0-2): 1069, 8228, 16993, 8354, 1148
https://tests.stockfishchess.org/tests/view/5ec0eaafe9d85f94dc429974

LTC:
LLR: 2.94 (-2.94,2.94) {0.25,1.75}
Total: 96552 W: 13946 L: 13498 D: 69108
Ptnml(0-2): 676, 9082, 28375, 9404, 739
https://tests.stockfishchess.org/tests/view/5ec145efe9d85f94dc4299b0

Bench 4395562

[mstembera]

Since lowPlyHistory is used in MovePicker it's not obvious to me why this didn't change bench. Does anyone know why not?

[pb00068]

@mstembera This change only affects game playing, not analysis of single positions. This is why bench does not change

[vondele]

can we use std::copy / std::fill to replace the loops (rather than memset/memcpy) ?

[xoto10]

The problem is, that is beyond my c++ abilities! I coded the simple loops as I thought the optimiser would make that pretty efficient and I understood it. :)

My guess at copy and fill is

+  std::copy(&lowPlyHistory[0][0], &lowPlyHistory[MAX_LPH - 2][0], &lowPlyHistory[2][0]);
+  std::fill(&lowPlyHistory[MAX_LPH - 2][0], &lowPlyHistory[MAX_LPH][0], 0);

but it changes the bench, so presumably something is wrong :( (And the reference says the third arg to copy shouldn't point into the range defined by the first 2 args, so I'm not sure what would happen if we increase the size of the array.)
Perhaps @pb00068 and @mstembera can help me out ... ?

Edit: the problem appears to be with the std::copy() but I haven't yet figured out how to fix it ...

[d3vv]

Pardon me for off-topic.. I suggest to use '\0' into std::fill always, cause:

$ cat memset_bzero_fill.cpp 
#include <cstdlib>
#include <algorithm>
#include <string.h>

int main(int argc, char* argv[]) {
  int mode = argc > 1 ? std::atoi(argv[1]) : 1;
  int n = 1024 * 1024 * 1024 * 1;
  char* buf = new char[n];
  if (mode == 1)
    memset(buf, 0, n * sizeof(*buf));
  else if (mode == 2)
    bzero(buf, n * sizeof(*buf));
  else if (mode == 3)
    std::fill(buf, buf + n, 0);
  else if (mode == 4)
    std::fill(buf, buf + n, '\0');
  return buf[0];
}

$ cat Makefile 
all: build run

.SILENT:

target = memset_bzero_fill

build:
	g++ -O2 -o $(target) $(target).cpp

run: run-memset run-bzero run-fill-1 run-fill-2

go:
	bash -c "(time -p ./$(target) $(mode))" 2>&1 | head -1 | cut -d' ' -f 2

run-memset:
	echo $@ `$(MAKE) go mode=1`

run-bzero:
	echo $@ `$(MAKE) go mode=2`

run-fill-1:
	echo $@ `$(MAKE) go mode=3`

run-fill-2:
	echo $@ `$(MAKE) go mode=4`

$ make
run-memset 0.79
run-bzero 0.77
run-fill-1 1.19
run-fill-2 0.77

[mstembera]

@xoto10 The order of source and destination arguments for std::copy is reversed compared to std::memcpy. Also you are right that for std::copy the source and destination should not overlap so we should use std::move instead. Here is what I have:

std::move(&lowPlyHistory[2][0], &lowPlyHistory[MAX_LPH][0], &lowPlyHistory[0][0]);
std::fill(&lowPlyHistory[MAX_LPH - 2][0], &lowPlyHistory[MAX_LPH][0], 0);

@pb00068 Thanks. I have always wished bench would actually play a few moves so it would catch changes like this.

@d3vv Interesting. Is this only because *buf is of type char? What if it was of type int or int16_t like our histories? Should we then use int16_t(0) to std::fill?

[d3vv]

@mstembera I don't know.. But with -O3 latest gcc will be good but not always.. And I have troubles with -O3 many years ago and found out this article:

http://demin.ws/blog/russian/2011/05/04/who-is-faster-memset-bzero-std-fill/

But this article is in Russian..

[d3vv]

I mean just the correct type for zero-symbol needed to help choose optimal template for binary code building..

[silversolver1]

Imo it is far clearer in its current state. leaving it this way would also make it more easily modifiable by other users in the future

[mstembera]

@silversolver1 We use stdlib methods like std::fill and others all over SF as it's more efficient, less code, and standardized. IMO this is easily within the minimum base level of C++ competence already required to understand existing SF code.

[vondele]

@d3vv with a recent compiler all options (except the call to bzero) compile to the same memset code:
https://godbolt.org/z/DVNg6D

[xoto10]

@mstembera
Thanks for pointing out that the args are the other way round - even when I read that last night I still didn't believe you and misread the reference page (again). But a night's sleep and some caffeine has aided the process :)
I've used std::copy and std::fill, squashed and pushed - maintainer can decide if this is ok.

[adentong]

appveyor failed. is this something we need to worry about?

[xoto10]

I think (hope!) this is what proton is addressing in #2653 (comment)

[protonspring]

Yeah. . I was bored, so I went through the build stuff and added casts as needed. I don't have MSVC, so I'm just guessing, but it should be close.

[vondele]

I think the appveyor failure is a random timeout. To be sure you can close and reopen the PR to trigger it again.

[vondele]

hmm, it looks like it is reproducible, so we'll have to figure it out prior to merging. From looking at the log, it is hanging at the point where it runs the binary. I don't have access to MSVC

[vondele]

Try

CXXFLAGS="-D_GLIBCXX_DEBUG" make -j ARCH=x86-64-modern build debug=yes optimize=no && ./stockfish bench

Error: attempt to subscript container with out-of-bounds index 4, but
container only holds 4 elements.

[...]
#3  0x000055c76584e50a in std::__debug::array<Stats<short, 10692, 4096>, 4ul>::operator[] (this=0x55c7670a0230, __n=4) at /usr/include/c++/9/debug/array:155
#4  0x000055c76583f04f in Thread::search (this=0x55c76709a850) at search.cpp:360
#5  0x000055c76583e34d in MainThread::search (this=0x55c76709a850) at search.cpp:246
#6  0x000055c7658613bd in Thread::idle_loop (this=0x55c76709a850) at thread.cpp:129

So, it looks like obtaining the address this way is not OK.

[xoto10]

I guess specifying the last element and adding 1 would be more intuitive. Anyway, let's see what happens ...

[mstembera]

@xoto10 Yes

std::copy(&lowPlyHistory[2][0], &lowPlyHistory.back().back() + 1, &lowPlyHistory[0][0]);
std::fill(&lowPlyHistory[MAX_LPH - 2][0], &lowPlyHistory.back().back() + 1, 0);

would be one way to do it but I have no preference.

[vondele]

I'll be merging patches tomorrow, so maybe time to look at one more version instead of back() + 1 use begin() / end() in a suitable way?

[mstembera]

I'm not sure if you wanted to get rid of back() completely? If so then

  std::copy(lowPlyHistory[2].begin(), lowPlyHistory[MAX_LPH - 1].end(), lowPlyHistory[0].begin());
  std::fill(lowPlyHistory[MAX_LPH - 2].begin(), lowPlyHistory[MAX_LPH - 1].end(), 0);

should also work.

[xoto10]

Yes, it seems there are various combinations. I was trying to use the functions as much as possible in this test, but somehow I didn't find front() when I looked for it. I think maybe your way is neatest, always use [] for the first dimension of the array and always use begin() or end() for the second.

[xoto10]

Oops, I forgot to wait for that earlier version to finish the checks! I think it was working ok. :)

[xoto10]

Ah, I expected that to work :( For the record, the code is

  std::copy(lowPlyHistory[2].begin(), lowPlyHistory[MAX_LPH - 1].end(), lowPlyHistory[0].begin());
  std::fill(lowPlyHistory[MAX_LPH - 2].begin(), lowPlyHistory[MAX_LPH - 1].end(), 0)

and it hung when running bench. Will revert to using back() and wait for the checks to finish this time ...

[xoto10]

It looks like end() doesn't work with MSVC, and I'm not sure there's much point in using begin() if we're not using end(). So I've retained the & operators and used back().back() + 1. Obviously I can change this if Joost prefers a different style.

[adentong]

Just need this to be merged before divP now.

[vondele]

Thanks!

[mstembera]

@xoto @vondele Should we not still clear the lowPlyHistory whenever we clear the TT?

[mstembera]

Never mind. Looks like we still do.