couple of questions about stockfish code ?

[Rein Halbersma]

What do you think of this:

Code: Select all

/// Score enum stores a middlegame and an endgame value in a single integer
/// (enum). The most significant 16 bits are used to store the endgame value
/// and the lower 16 bits are used to store the middlegame value. Take some
/// care to avoid undefined behavior (such as left-shifting a negative int)
/// and relying on implementation-defined behavior (such as casting to signed
/// ints).
enum Score : uint32_t { SCORE_ZERO };

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

/// Make sure the platform is reasonable.
static_assert(uint16_t(INT16_MIN) == INT16_MAX + 1, "Range of int16_t is unsupported.");

/// Helper function for correctly casting uint16_t to int16_t.
/// The compiler will optimize this to a no-operation.
inline int16_t cast_to_int16_t(uint16_t v) {
  return v <= INT16_MAX ? int16_t(v) : int16_t(v - INT16_MIN) + INT16_MIN;
}

inline Value eg_value(Score s) {
  return Value(cast_to_int16_t((s + 0x8000U) >> 16));
}

inline Value mg_value(Score s) {
  return Value(cast_to_int16_t(s));
}

I would just use the straightforward operator overloading on a plain

Code: Select all

 struct Score { int mg, eg }; 

It's not just the packing that causes me headaches, also the 16-bit integer promotions that are very hard to get right. If it works for you, great, but it's a maintenance nightmare IMO. I'd happily sacrifice 5 ELO for readability and a guarantee of correctness.

[syzygy]

I would just use the straightforward operator overloading on a plain

Code: Select all

 struct Score { int mg, eg }; 

But that is a major slowdown.

It's not just the packing that causes me headaches, also the 16-bit integer promotions that are very hard to get right. If it works for you, great, but it's a maintenance nightmare IMO. I'd happily sacrifice 5 ELO for readability and a guarantee of correctness.

It is hard to get right mainly because it is hard to get wrong. Most things you do will just work with all compilers even if it is formally UB or relying on implementation-defined behavior. Of course there are still good reasons for trying to get it "right", especially if that is possible with simple and efficient code.

The maintenance problem I don't really see. There is necessarily some complication when packing two values in one int, but it is all beatifully hidden behind an unproblematic interface.

Anyway, I have removed the static assert and simplified cast_to_int16_t to:

Code: Select all

inline int16_t cast_to_int16_t(uint16_t v) {
  return v < 0x8000 ? v : v - 0x10000;
}

Just shift the value modulo 2^16 to be within the range of int16_t, then cast.

For correctness this relies on ints being wider than 16 bits (or would the presence of 0x10000 otherwise still save the day, I'm not sure), but SF already assumes an int to be at least 32 bits. In the end C and C++ were never designed to be "write once, run anywhere".

[Rein Halbersma]

The only thing in which we agree is "you should test and measure" that contrasts pretty badly with your "I'm pretty sure using a struct of two 16-bit ints would be measurably slower"

"Pretty sure" allows for the possibility that I turn out to be dead wrong, in which case I will simply have to admit that and will do so. But I don't think you'll prove me wrong here. And I'm talking about Stockfish, not about a simple loop that does not suffer from register pressure.

The reason for being "pretty sure" is that a single register for holding 1 value is pretty certain to be more efficient than two registers for holding 2 values. And while it is true that modern CPUs can perform many operations in parallel, reducing the number of operations is not going to hurt and will leave execution units free for performing other operations.

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.

I hope we don't get a discussion now on whether a 3.5% slowdown should be somehow acceptable for a program like Stockfish just to make a few lines of code in types.h perhaps easier to read. (Code that one does not even need to understand when working on any other part of Stockfish.)

Is it still +3.5% if you compile with -freg-struct-return ?

[syzygy]

Is it still +3.5% if you compile with -freg-struct-return ?

I think so, because almost all of the evaluation functions returning Score structs should be getting inlined anyway. But I will give it a try.

edit: yes, still 3.5% on bench 128 1 20.

[Fulvio]

I think so, because almost all of the evaluation functions returning Score structs should be getting inlined anyway.

Just to be 100% sure, can you please also test the current implementation wrapped inside a struct:

Code: Select all

struct Score {
  int32_t data_;

  inline Score operator+(const Score& a) const {
    return { int32_t(data_ + a.data_) };
  }

  inline Score operator-(const Score& a) const {
    return { int32_t(data_ - a.data_) };
  }

  inline Score& operator+=(const Score &a) {
    data_ += a.data_;
    return *this;
  }

  inline Score& operator-=(const Score &a) {
    data_ -= a.data_;
    return *this;
  }

  inline Score operator*(const int a) const {
    return { int32_t(data_ * a) };
  }

  inline Score operator-() {
    return { int32_t(-data_) };
  }
};

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

const Score SCORE_ZERO = make_score(0, 0);

inline Value eg_value(Score s) {
  union { uint16_t u; int16_t s; } eg = { uint16_t(unsigned(s.data_ + 0x8000) >> 16) };
  return Value(eg.s);
}

inline Value mg_value(Score s) {
  union { uint16_t u; int16_t s; } mg = { uint16_t(unsigned(s.data_)) };
  return Value(mg.s);
}

[BeyondCritics]

http://stackoverflow.com/questions/1315 ... d-behavior

See the top answer. This leads to the following solution (I don't think there is any reason to use static_cast here, but I don't speak C++):

In C++ static_cast is preferred because of 1) greppability, and 2) in general C-style casts might do a reinterpret_cast under the covers (not in this case, but see #1). It just makes the intent explicit, so it's a good idiom to use.

Yes, see here: http://en.cppreference.com/w/cpp/language/explicit_cast
Even better should be to hide the cast within a dedicated method, e.g.:

Code: Select all

inline int as_int(Value v) { return static_cast<int>(v);} 

[syzygy]

http://stackoverflow.com/questions/1315 ... d-behavior

See the top answer. This leads to the following solution (I don't think there is any reason to use static_cast here, but I don't speak C++):

In C++ static_cast is preferred because of 1) greppability, and 2) in general C-style casts might do a reinterpret_cast under the covers (not in this case, but see #1). It just makes the intent explicit, so it's a good idiom to use.

Yes, see here: http://en.cppreference.com/w/cpp/language/explicit_cast
Even better should be to hide the cast within a dedicated method, e.g.:

Code: Select all

inline int as_int(Value v) { return static_cast<int>(v);} 

Why is that better if greppability is the point (or one of the points) of using static_cast?

If you can do int(v) or (int)v, why prefer as_int(v) instead? And I still don't see an advantage in using static_cast<int>(v) as it is just a lot more typing.

So it seems to me this is all just a matter of personal preference.

[syzygy]

I think so, because almost all of the evaluation functions returning Score structs should be getting inlined anyway.

Just to be 100% sure, can you please also test the current implementation wrapped inside a struct:

The performance is virtually identical to SF's current implementation.

[BeyondCritics]

Code: Select all

inline int as_int(Value v) { return static_cast<int>(v);} 

Why is that better if greppability is the point (or one of the points) of using static_cast?

You have the ugly cast well contained at one place.

If you can do int(v) or (int)v, why prefer as_int(v) instead?

Because it is fully type safe. int(v) is not.

And I still don't see an advantage in using static_cast<int>(v) as it is just a lot more typing.

Yes, there is case for the shorter variant here.

So it seems to me this is all just a matter of personal preference.

Well, not really. It is an established recommendation. See e.g. here
https://github.com/isocpp/CppCoreGuidel ... asts-named

[syzygy]

Code: Select all

inline int as_int(Value v) { return static_cast<int>(v);} 

Why is that better if greppability is the point (or one of the points) of using static_cast?

You have the ugly cast well contained at one place.

Yes, and then you can use it everywhere without it being greppable. Does that help? How does hiding something help the programmer realise the dangers of what he is doing?

I'll try to think "for your side". I suppose this is not about readability but about forcing the compiler to do some extra checking. I suppose static_cast<int>(...) will trigger compilation errors where int(...) does not, probably depending on the type of the expression that is being cast.

Then I wonder a bit why C++ allows C-type casts at all (perhaps to improve compatibility with C) and even provides an arguably more convenient "function"-notation for it (definitely not of any help for improving compatibility with C).

I definitely like the C philosophy better than the C++ one. If I know that a cast is correct and what I want, just let me do it. By typing (int) I tell the compiler what I want it to do. But OK, C++ is C++.

(What I don't understand is how the C++ style can be reconciled with things like function and operator overloading, ideal devices for avoiding that the programmer knows what he is doing.)

Well, not really. It is an established recommendation. See e.g. here
https://github.com/isocpp/CppCoreGuidel ... asts-named

Those are just two guys trying to tell me what is good for me

In any event they are just recommendations and have to be understood rather than taken as gospel. So if I am writing a function cast_to_int16_t() with the sole purpose of avoid implementation-defined behavior when converting from uint16_t to int16_t, I really don't see a benefit in using static_cast.