couple of questions about stockfish code ?

[kbhearn]

that does bring up an improvement though - if the bitwise xor off of the sign bit happens before the conversion to int, then even the most exotic representations of 32 bit integer would work.

[BeyondCritics]

Uh..., i have to admit defeat.
After working through your example i had a small crisis, but now i am fully recovered with a much deeper understanding of the language.
Now i claim that i can prove, that (with or without your improvement) the implementation shown is 100% conforming and implementation neutral.
But there is still a problem: You have silently changed the representation and we were not told why this is allowed regarding the other operations defined. Therefore i checked it and unfortunately there is a problem with division.
Example:

Code: Select all

mg(make_score(0,-2)/2) == mg((2^16-2)/2) == mg(2^15-1) == 2^15-1 != 1

[Fulvio]

https://github.com/syzygy1/Stockfish/co ... ore_struct

The slowdown is about 3.5% on my system.

Please let me know if the operators can somehow be implemented more efficiently.

Thanks, this is interesting.
I've tried the code, but Visual Studio 2015 is not able to inline the operator functions (maybe it's because of the templates?).
Which compiler did you use?
Can you please test the same interface with all the operator overloads, but with a struct containing a single int32_t?
If the interface is transparent it should perform like the enum version.

[syzygy]

i'm not sure that that int16_t(...) cast is well defined - i believe it's implementation defined and what SF's union is trying to dodge since it's a cast of a value out-of-range of the target type casted to a signed type (which all would-be negative values would be out of range positive values instead).

It relies on implementation-defined behaviour, but is there any way to get around that?

Ah wait... that is why you did the sign extension manually! Now I get it

You are right that int16_t is guaranteed to be 2 bytes wide, have no padding bits, and use 2's complement. But I think there is no guarantee on the order of the bytes or even bits, so a compiler could choose to store uint16_t as litle endian and int16_t as big endian, breaking the union trick completely. This is very unrealistic, but so is the assumption that the int16_t() cast will not work (it should even work if the compiler mixes endianness as it must map the values within range correctly... but of course it could check for overflow and do something funny in that case).

So with either approach there is a reliance on implementation-defined behaviour. This seems acceptable to me. All relevant compilers define the behaviour in a way that causes no problems. (I think there will only ever be a problem if the hardware architecture is really funny compared to what we're used to nowadays... and then several things in SF would probably have to be adapted to that hardware anyway, if only for efficiency reasons.)

The union trick seems to have a somewhat bigger problem in C++11 in that the language standard seems not to allow it. The major compilers probably all do allow it, though (at least g++ does).

The "proper" way to implement the union trick might be this (untested and assuming Score to be uint32_t):

Code: Select all

inline Value eg_value(Score score) {

  int16_t s;
  uint16_t u = (s + 0x8000U) >> 16;
  memcpy(&s, &u, sizeof(int16_t));
  return Value(eg.s);
} 

inline Value mg_value(Score score) {

  int16_t s;
  uint16_t u = score;
  memcpy(&s, &u, sizeof(int16_t));
  return Value(s);
}

This avoids UB but still relies on implementation-defined behaviour: identical order of bits and bytes in int16_t and uint16_t.

Is your manual sign-shifting solution completely free of implementation-defined behaviour? Probably it is...

[syzygy]

http://stackoverflow.com/questions/9220 ... onversions

Ouch...

So the compiler could, and it seems come compilers do implement (int16_t)s as (s & 0x7fff). I would assume though that a decent compiler fills in the implementation-defined parts of the standard in a decent way...

[syzygy]

Uh..., i have to admit defeat.
After working through your example i had a small crisis, but now i am fully recovered with a much deeper understanding of the language.
Now i claim that i can prove, that (with or without your improvement) the implementation shown is 100% conforming and implementation neutral.
But there is still a problem: You have silently changed the representation and we were not told why this is allowed regarding the other operations defined. Therefore i checked it and unfortunately there is a problem with division.
Example:

Code: Select all

mg(make_score(0,-2)/2) == mg((2^16-2)/2) == mg(2^15-1) == 2^15-1 != 1

SF overloads the division operator for Score to make division by an integer work. (But I think this is never actually used in current SF.)

You can also interchange the order of eg and mg in the Score representation and everything will continue to work. This was in fact done recently and the comment explaining make_score() has not yet been adapted accordingly:

Code: Select all

/// Score enum stores a middlegame and an endgame value in a single integer
/// (enum). The least significant 16 bits are used to store the endgame value
/// and the upper 16 bits are used to store the middlegame value. Take some
/// care to avoid left-shifting a signed int to avoid undefined behavior.
enum Score : int { SCORE_ZERO };

inline Score make_score(int mg, int eg) {
  return Score((int)((unsigned int)eg << 16) + mg);
}

[BeyondCritics]

...
Example:

Code: Select all

mg(make_score(0,-2)/2) == mg((2^16-2)/2) == mg(2^15-1) == 2^15-1 != 1

SF overloads the division operator for Score to make division by an integer work.
...

I know that it overloads the operator, otherwise division would not compile.
I think the problem here, is that the new encoding introduces a nonlinear transformation for negative arguments and that makes division cease to work.

...(But I think this is never actually used in current SF.)...

So you know the whole stockfish? Impressive...

[syzygy]

The current Score extraction functions of Stockfish seem to carefully avoid direct casts from uint16_t to int16_t (by using the union-trick), but the same is not true for make_score():

Code: Select all

enum Score : int { SCORE_ZERO };

inline Score make_score(int mg, int eg) {
  return Score((int)((unsigned int)eg << 16) + mg);
}

A direct cast from unsigned int to int, which allows a very silly compiler to zero the most-significant (sign) bit. (And the extra (int) is of course superfluous.)

This could be made cleaner by specifying the underlying type of Score as uint32_t.

Or it could be accepted that there is reliance on direct casts from unsigned to signed working properly, in which case the union trick is no longer necessary.

[syzygy]

...
Example:

Code: Select all

mg(make_score(0,-2)/2) == mg((2^16-2)/2) == mg(2^15-1) == 2^15-1 != 1

SF overloads the division operator for Score to make division by an integer work.
...

I know that it overloads the operator, otherwise division would not compile.
I think the problem here, is that the new encoding introduces a nonlinear transformation for negative arguments and that makes division cease to work.

Maybe I am missing something, but as far as I understand division and multiplication of two Score values is not intended and (hopefully) leads to a compilation error.

Division by an integer is defined component-wise. The eg and mg are extracted from the Score value, they are divided by the integer, and a new Score value is constructed using make_score(). Whatever is changed in the internal representation of Score is not going to affect this.

...(But I think this is never actually used in current SF.)...

So you know the whole stockfish? Impressive...

In Cfish I included:

Code: Select all

INLINE Score score_divide(Score s, int i)
{
  return make_score(mg_value(s) / i, eg_value(s) / i);
}

but I ended up never using score_divide() in the rest of the code. This means the operator overload is never used, because if I had overlooked it Cfish's node counts would deviate from those of Stockfish.

[BeyondCritics]

After looking that up here, i see that vectorized division of Scores is actually not defined in my Stockfish and it wouldn't work either. Sorry for the confusion i created. I had i wrong in memory from yesterday.