Stockfish: Incorrect bench with no clear cause

[silversolver1]

Hello.

I am opening this issue primarily in reference to this test being stopped: FishTest.

I have checked the bench multiple times (including rebuilding) and I continually get the same result (5193629), which is said to be incorrect.

Can anyone help me understand what I am doing wrong?

The patch in question is this one: https://github.com/silversolver1/Stockfish/commit/d3fde0b0f998cc1b374d3febfbf1627722f228e6

Additional Details:

Compiled by g++ (GNUC) 9.2.0 on MinGW64
__VERSION__ macro expands to: 9.2.0

I have been using in essence the following .sh file to build Stockfish:

#!/bin/bash
# custommakefish.sh

cd Stockfish-master/src

arch_cpu=x86-64

#make build ARCH=x86-64
make -j profile-build ARCH=${arch_cpu} COMP=mingw
strip stockfish.exe
mv stockfish.exe ../../stockfish_local_modified_${arch_cpu}.exe 
make clean
cd

I am on 64 bit Windows using the MSYS2 MinGW 64-bit shell and this has been my primary source of information: Build SF Guide

Thanks.

[vondele]

that's a common trap I think... attacks_bb is initialized by the statement in evaluate.cpp near line 803. The order of evaluation in the sum is not specified, and depends on the compiler, so if you use the attacks_bb as part of this statement, it will return seemingly random results.

[silversolver1]

@vondele One more question, if you can: do you have any ideas as to how I could avoid this problem in the future? I'm not exactly sure if I am understanding this properly. I can't tell if the problem is with my implementation of the patch or if this is a problem inherent to attacks_bb.

[vondele]

basically, if you want to use attacks_bb, you have to use it later, i.e. after pieces are evaluated, for example in king, threads, passed, space... maybe @Vizvezdenec can be help you with what you want to achieve.

[Vizvezdenec]

Vondele is slightly wrong.
It's not about attacks_bb. Attacks_bb can be used there.
(look at line 270).
It's about & attackedBy[Them][ALL_PIECES]) - which depends on what opponent pieces were already looped thru in piece cycle, so basically depends on compiler (and also it depends on which color we are - since if it's white cycle black attackedBy[all_pieces] is empty).
But this patch overall seems to have faulty logic :)
attacks_bb is used to create bitboard of certain attacks blocked by pieces that are in ().
Look at line 270 for example. So your patch gives penalty if any square xray attacked by queen but stopped by our pawn is attacked by any their piece... Kinda makes pretty low sence.
If you want to give extra penatly for pawns attacked by our queen and attacked by their pieces you just should do smth like this https://github.com/official-stockfish/Stockfish/blob/master/src/evaluate.cpp#L522 - but to be honest this is already done by this eval term.

[Vizvezdenec]

Actually thinking one more time - it's probably due to color assymetry mostly. Since we use attackedBy[Them][ALL_PIECES] - it either has only pawn and king attacks OR has attacks of all pieces, so it depends in which order compiler forces stuff to be done - first white than black or first black than white.
Anyway it does it wrong since one of this bitboards is not really formed. Actually this is pretty creative way to fall into "partially formed bitboard" trap, I haven't seen exact this way by far I think :)

[silversolver1]

@vondele @Vizvezdenec Thanks for the advice. I'll try and make things more clear to the compiler in future patches.

So it essentially boils down to this: depending on how a particular compiler handles attackedBy[Them][ALL_PIECES], at least when used where and how I used it, the bench result could be different?

Is there any way to define this or have this defined more explicitly instead of leaving the decision up to a particular compiler? Or maybe we could use an assert of some kind to make it more likely to catch an error like this at compilation time? Not really sure how viable that last suggestion is though.

Also thanks for taking the time to explain it to me by the way. I appreciate it :)

My next test of this will probably involve ~attackedBy[Them][QUEEN] instead of attackedBy[Them][ALL_PIECES] so hopefully that is handled essentially the same way by most compilers instead of differently like this one was

Also another reason more generally that this incorrect bench result was strange is because at the same time I submitted this particular patch, I had also submitted two of essentially the exact same ones but with different values: One and Two. Both of these went through perfectly fine and finished without issue.

The initial one that was stopped is here: Three

[vondele]

It depends on the order the compiler evaluates the terms in:

    score +=  pieces<WHITE, KNIGHT>() - pieces<BLACK, KNIGHT>()
            + pieces<WHITE, BISHOP>() - pieces<BLACK, BISHOP>()
            + pieces<WHITE, ROOK  >() - pieces<BLACK, ROOK  >()
            + pieces<WHITE, QUEEN >() - pieces<BLACK, QUEEN >();

one could enforce a particular order by splitting this expression and by having first white then black, first queen, etc.

Most workers are running with one particular version of gcc, so the order is nearly always the sma. It just depends on what workers you get, that use different compilers.

[silversolver1]

@vondele Ok I see. Do you think this would be a reasonable change to make? It might change the bench initially but from that point onwards if it made things more consistent across the board it could potentially be worthwhile (meaning we leave it less up to chance that things would work out correctly).

I also tried to test it out what I imagined an implementation would be like on godbolt but I'm not really familiar enough with godbolt to get it to work :')

[Vizvezdenec]

@silversolver1
answer to your questions.
it means that pieces() loop is used to form this bitboards.
line 278 says attackedBy[Us][Pt] |= b; - this is where attacked by currently looped piece type is created properly.
So if you use attackedBy[Them][QUEEN] or stuff like this if we are WHITE than BLACK attackedBy[QUEEN] bitboards are not yet created so they are assigned some random stuff.
Simply put - you can't really use attackedBy[][] apart from attackedBy[PAWN] or attackedBy[KING] in pieces().
Also you seem to misunderstand what attacks_bb(s, bitboard) does.
It forms a bitboards of XRAY attacks from s square by QUEEN (or other piece type in template) with bitboard being stoppers.
For example if s is a2 square and bitboard = a4 d5 d8 squares this will be a1 a3 a4 b3 c4 d5, all 2nd row and b1. If you want pawns attacked by the queen you should use attackedBy[Us][QUEEN] although it's also not really good to use it in pieces().

[silversolver1]

Ok so I think I figured out a way to solve this problem. We could do something like this:
in Evaluation::value, we would have to find a way to check whose side it is to move and what color they are playing as currently.

One way to do this is to use the bool pos.side_to_move() == WHITE

If true, we would initialize black first and populate its attack tables using pieces. then we would do the same for white.
if false, we would we would do the opposite: initialize white first, its attack tables and then the same for black.

this could open up possibilities for more functionality regarding attackedBy in pieces.

I attempted to write something that could maybe work in the following PR: https://github.com/silversolver1/Stockfish/commit/e75ba076c2ccb42647e5ae28d0a3430580b93691
but as my compiler is telling me, there are many, many reasons that the following code won't work as is so it is basically more just pseudocode at this point. It will take more than the changes i have made in that PR to actually get it to work. it's a start though so hopefully we can figure something out

edit: note that what i wrote is contingent on the idea that pieces, mobility, king, threats, passed and space do not need the enemy side to be initialized just yet. if they do then this probably won't work :)

edit 2: this is also assuming as easier solution isn't just to say if pos.side_to_move() == WHITE, just do black first then white. and if pos.side_to_move() == BLACK just do white first then black. i tried to create a solution that would essentially allow us to use variables instead of having to duplicate the code

edit 3: i realized another way we could do it is create array of type Color holding both the first and second sides to move. we could then have it so that instead of having to use separate variable entirely for first side and second side, we could just use a loop and an reference to the colors stored in the array. i'll see if i can write something like that that up

edit 4: i wrote it up here https://github.com/silversolver1/Stockfish/commit/e93e45710f22722ba30a80dcbd96e631a6c6dcc3. it is far cleaner and more compact. could actually potentially be useful :)

[silversolver1]

One example of bypassing this issue can be see in: https://github.com/official-stockfish/Stockfish/compare/master...praveentml:pbu3, which is @praveentml's patch. it hasn't passed stc yet so of course no guarantee of commit but i think the idea is promising.

my comment on there is essentially as follows:
"i thought this patch was a really cool way of avoiding the problem of uninitialized bitboards that can sometimes unintentionally happen when working in Evaluation::pieces(). taking it directly to Evaluation::threats() (which happens after the enemy pieces have already been initialized) is a very clever way of bypassing this issue and in fact would open up a lot of functionality that was not available previously (like being able to more easily take enemy pieces into account)."

[Rocky640]

@silversolver1
I think you are overthinking this.
it is quite simple, and should be kept simple.

first we populate the attackedBy, attackedBy2
```
score += pieces() - pieces()
+ pieces() - pieces()
+ pieces + pieces

then we do something with them, and consider more complex interactions

score +=  king<   WHITE>() - king<   BLACK>()
        + threats<WHITE>() - threats<BLACK>()
        + passed< WHITE>() - passed< BLACK>()
        + space<  WHITE>() - space<  BLACK>();

score += initiative(score);

```

"threats" is normal place to add new stuff which requires the full attack bitboard initialized.

There were a few attempts to calculate all the attackedBy, attackedBy2 first,
and only after this calculate the pieces, but that would duplicate the piece loop,
and the slow down would cancel out any "benefit".

Finally, you will notice that sidetomove is never used in evaluation.
Many year ago all they were gradually cleaned up, and sine then, sparse attempts to resurrect them failed.

[silversolver1]

@Rocky640 Yes, I was most definitely overthinking this at the time. I merely added the comment earlier today to document it for posterity's sake as I thought @praveentml's solution was useful. It was also not something I had initially considered. Also I have seen at least one person other than me have a very similar issue in Fishtest in just the past few days so again, I figured documenting my thoughts and a potential solution could be useful if anyone decided to search for it.

As for whether it is worth it or not, I have not yet tried submitting that small patch I wrote (or an equivalent version) just yet. There may be a trade-off but, (assuming it passes all the necessary testing), it could be worth it if it means we actually ensure a consistent bench across all workers, regardless of what compiler they have. This is currently not the case and in my opinion undesirable.

Admittedly, the issue can only arise because of the logic error that I initially made but nonetheless no visible errors in compilation show or occur and there is simply no way for the user to even know they are doing something wrong before submitting the test. And even then, unless they happen to get that particular worker using that particular compiler that gives a different bench, it is possible that this logic error will not trigger any red flags at all. Consider how I submitted 3 essentially equivalent tests (only value differences) and 2 out of 3 of them successfully finished STC testing. It would not be unreasonable to say that on the off chance that one of the tests somehow got through LTC, the error might end up being caught only at the Travis/AppVeyor stage.

It is most likely better for us to try and build safety into the system rather than try and rely on the user to not make a particular mistake.

Some other avenues we can consider might include asserts, boolean flags, or perhaps more detailed/specific comments/warnings in some way

[vondele]

@Rocky640 I have added this to the cleanups https://github.com/official-stockfish/Stockfish/pull/2628/commits/43a8acbf59bb1017e8b635c4b388c2ecdccb236b let me know if the wording can be improved.

[vondele]

concerning side_to_move not being used in eval, there are actually exceptions to this in endgame.cpp.
E.g.

 else if (   distance(bksq, psq) >= 3 + (pos.side_to_move() == weakSide)
           && distance(bksq, rsq) >= 3)
      result = RookValueEg - distance(wksq, psq);

And indeed the idea to make Tempo dependent on the actual position (dynamic) seems natural, but never worked so far.

[vondele]

I'll close this, since the improved comments have now been merged in master.