gcc miscompilation in Stockfish 1.6

[Gerd Isenberg]

There is nothing in the C standard that guarantees that. See section 6.5.7, point 5:

http://www.open-std.org/JTC1/SC22/WG14/ ... /n1256.pdf

Ok, so what about this one ?

Code: Select all

inline Value eg_value(Score s) { return Value((int)(unsigned(s) & 0x7fffu) - (int)(unsigned(s) & 0x8000u)); }

I have verified it works with all the compilers and does not rely on shifts or cast to shorts. I have found it googling around and it seems safe to me.

And how much do we now save, compared to actually storing two separate values of type short?

Are compiler able to keep short[2] inside registers? I found horrible generated assembly while using shorts, so I guess it is worth the effort. However I would prefer real SWAR ...

[Bo Persson]

There is nothing in the C standard that guarantees that. See section 6.5.7, point 5:

http://www.open-std.org/JTC1/SC22/WG14/ ... /n1256.pdf

Ok, so what about this one ?

Code: Select all

inline Value eg_value(Score s) { return Value((int)(unsigned(s) & 0x7fffu) - (int)(unsigned(s) & 0x8000u)); }

I have verified it works with all the compilers and does not rely on shifts or cast to shorts. I have found it googling around and it seems safe to me.

And how much do we now save, compared to actually storing two separate values of type short?

Exactly what I wanted to say, right in this minute. I think the whole issue has gone too far into bit twiddling "at its best" while not being necessary at all.

I just renew my "class Score" proposal. This one compiles under GCC and includes everything you would really need. There is no bit operation involved, just two 16-bit values are kept and operated upon simultaneously. The user code needs a little bit of editing but I think it should be worth the small effort:

Code: Select all

class Score {
public:
    // constructors
    Score() : mg(0), eg(0) {}
    Score(int mg_val, int eg_val) : mg(mg_val), eg(eg_val) {}
    Score(int eg_val) : mg(0), eg(eg_val) {} // maybe subject of discussion

    // accessors
    int mg_value() const { return int(mg); }
    int eg_value() const { return int(eg); }

    // unary minus operator
    Score operator-() { return Score(-int(mg), -int(eg)); }

    // binary member operators
    Score operator+(Score s) { return Score(mg + s.mg, eg + s.eg); }
    Score operator-(Score s) { return Score(mg - s.mg, eg - s.eg); }
    Score operator+=(Score s) { mg += s.mg; eg += s.eg; return *this; }
    Score operator-=(Score s) { mg -= s.mg; eg -= s.eg; return *this; }

    // binary friend operators
    friend bool operator==(Score const & s1, Score const & s2)
        { return s1.mg == s2.mg && s1.eg == s2.eg; }
    friend bool operator!=(Score const & s1, Score const & s2) { return !(s1 == s2); }
    friend Score operator*(int i, Score s) { return Score(i * s.mg, i * s.eg); }
    friend Score operator*(Score s, int i) { return i * s; }

    // Division must be handled separately for each term
    friend Score operator/(Score s, int i) { return Score(s.mg / i, s.eg / i); }

    // Only declared but not defined. We don't want to multiply two scores due to
    // a very high risk of overflow. So user should explicitly convert to integer.
    friend Score operator*(Score s1, Score s2);

private:
    int16_t mg;
    int16_t eg;
};

Maybe someone comes up with something even better?

Sven

Looks good to me.

Possibly you could make the constructor from int explicit, to avoid unwanted implicit conversions.

explicit Score(int eg_val) : mg(0), eg(eg_val) {}

[Bo Persson]

There is nothing in the C standard that guarantees that. See section 6.5.7, point 5:

http://www.open-std.org/JTC1/SC22/WG14/ ... /n1256.pdf

Ok, so what about this one ?

Code: Select all

inline Value eg_value(Score s) { return Value((int)(unsigned(s) & 0x7fffu) - (int)(unsigned(s) & 0x8000u)); }

I have verified it works with all the compilers and does not rely on shifts or cast to shorts. I have found it googling around and it seems safe to me.

And how much do we now save, compared to actually storing two separate values of type short?

Are compiler able to keep short[2] inside registers? I found horrible generated assembly while using shorts, so I guess it is worth the effort. However I would prefer real SWAR ...

I'm not sure about gcc, but I have seen that MSVC generates identical code (and actually shares the generated code) for these two vectors:

Code: Select all

struct two_shorts
{
   short x;
   short y;
};

std&#58;&#58;vector<int>  v1;
std&#58;&#58;vector<two_shorts>   v2;

[mcostalba]

Looks good to me.

Possibly you could make the constructor from int explicit, to avoid unwanted conversions.

explicit Score(int eg_val) : mg(0), eg(eg_val) {}

yes, I would also think two int are better then two shorts. I think I will test this approach in the new SF development, but for the ready to be released 1.6.2 I would try to keep changes at a minimum also performance wise and re-testing performance of this for all the platforms is too much and is _not_ the correct timing IMHO. Now we should try to get out 1.6.2 with NO functionality changes and NO performance changes so that all the work done by the testers till now is not wasted.

Sven approach is very interesting but is new development cycle material IMHO, now I would go with what we already have in 1.6 + minimal fixes for making gcc -O3 compile to work.

[Sven]

Looks good to me.

Possibly you could make the constructor from int explicit, to avoid unwanted conversions.

explicit Score(int eg_val) : mg(0), eg(eg_val) {}

yes, I would also think two int are better then two shorts. I think I will test this approach in the new SF development, but for the ready to be released 1.6.2 I would try to keep changes at a minimum also performance wise and re-testing performance of this for all the platforms is too much and is _not_ the correct timing IMHO. Now we should try to get out 1.6.2 with NO functionality changes and NO performance changes so that all the work done by the testers till now is not wasted.

Sven approach is very interesting but is new development cycle material IMHO, now I would go with what we already have in 1.6 + minimal fixes for making gcc -O3 compile to work.

Perfectly fine for me, there is absolutely no reason to hurry at the wrong time

Sven

[Jan Brouwer]

How is Value exactly defined?
I didn't find in this thread (maybe I missed it).

Jan

It is defined just few lines above Score in value.h file

Code: Select all

enum Value {
  VALUE_DRAW = 0,
  VALUE_KNOWN_WIN = 15000,
  VALUE_MATE = 30000,
  VALUE_INFINITE = 30001,
  VALUE_NONE = 30002
};

In that case, Value variables can be have different sizes depending on the compiler (implementation defined).
Interestingly, it seems that according C99, (unsigned) short is _not_ amongst the allowed sizes, but is allowed by the C++ standard.

[Sven]

yes, I would also think two int are better then two shorts.

But this would involve passing around 8 bytes by value instead of 4 quite frequently.

"Trust your compiler"

Sven

[mcostalba]

How is Value exactly defined?
I didn't find in this thread (maybe I missed it).

Jan

It is defined just few lines above Score in value.h file

Code: Select all

enum Value {
  VALUE_DRAW = 0,
  VALUE_KNOWN_WIN = 15000,
  VALUE_MATE = 30000,
  VALUE_INFINITE = 30001,
  VALUE_NONE = 30002
};

In that case, Value variables can be have different sizes depending on the compiler (implementation defined).
Interestingly, it seems that according C99, (unsigned) short is _not_ amongst the allowed sizes, but is allowed by the C++ standard.

Anyhow in this case I would rule out Value to be implemented as short because you have data above 30000 to store, so it could be or int (and I would bet it is) or unsigned if the compiler wants to really hurt itself (and its users).

If the compiler is really really evil it coudl try with unsigned short (because it's C++)...but I don't think compilers are so evil

[mcostalba]

yes, I would also think two int are better then two shorts.

But this would involve passing around 8 bytes by value instead of 4 quite frequently.

"Trust your compiler"

Sven

Yes I trust, but I also see that we have a lot of adds of the form

Code: Select all

eval += bonus; // both eval and bonus are Score

where the current implementation is just one add, with two integers is the add of two integers, with two shorts perhaps it is a bit slower, but of course tests is needed.