gcc miscompilation in Stockfish 1.6

[mcostalba]

yes typedef of int could be a very good idea, but I would like to intercept divsion by an integer value:

Code: Select all

// Division must be handled separately for each term
inline Score operator/(Score s, int i) { return make_score(mg_value(s) / i, eg_value(s) / i); }

I don't think I can do with a typedef .....

Don't bury this kind of special handling within a standard division operator. Use a normal function for it instead. Otherwise you are creating code which can be hard to understand for others. Special semantics should be visible to the reader if possible.

And you are also about to create weird effects somewhere else in the program. Suppose you have an expression "int s = some_value / 2;" and later on you decide to change the type of "some_value" to "Score", what do you expect to happen then? You probably won't notice it (maybe you get a compiler warning which annoys you and you add a cast to int) but the result will be surprising. Using operators can be nice sometimes but this is not a good example IMO.

Sven

Actually I would think that operators are used mainly to avoid difficult to spot errors.

In this case it is up to the compiler to check for all the Score variables and divide them in the proper way. If I use a function score_divide() I can miss some place and I can forget some: "int s = some_value / 2;" where some_value is a Score or becomes a Score, according to your example.

If I use a function then "int s = some_value / 2;" will go unoticed and the compiler will do the wrong thing, instead if I use an operator I will be assured that some_value / 2 will be divided correctly as long as _all_ the Score variables have to be divided in that special way (and this is true).

So, although I agree on your starting points, my conclusions seems the opposite IMHO the division operator is useful exactly to avoid some hidden bug because compiler will catch for us all the divions of a Score variable, instead with a function _you_ have to do the job.

As a bottom line I think use of operators enforces type checking, more then ad hoc functions. So it is not only a "nice" thing but more a "safe" thing to do....of course IMHO.

[Sven]

Maybe you can try something like this instead of "enum Score { WHATEVER }", although it is a little bit dangerous, too:

Code: Select all

class Score {
public:
    Score() : v(0) {}
    Score(int val) : v(val) {}
    operator int() { return int(v); }
private:
    int32_t v;
};

The problem is the constructor taking one int as argument. This may lead to some integers being converted to a Score instance by the compiler if he likes to, although I think this is hardly possible without being detected. I have checked that it compiles with MS Visual Studio based on the current public SF 1.6 code but I can't check whether it works, nor do I have a GCC installed right now.

There should be no additional overhead, and the code should be portable, too. Give it a try ...

Sven

[Sven]

yes typedef of int could be a very good idea, but I would like to intercept divsion by an integer value:

Code: Select all

// Division must be handled separately for each term
inline Score operator/(Score s, int i) { return make_score(mg_value(s) / i, eg_value(s) / i); }

I don't think I can do with a typedef .....

Don't bury this kind of special handling within a standard division operator. Use a normal function for it instead. Otherwise you are creating code which can be hard to understand for others. Special semantics should be visible to the reader if possible.

And you are also about to create weird effects somewhere else in the program. Suppose you have an expression "int s = some_value / 2;" and later on you decide to change the type of "some_value" to "Score", what do you expect to happen then? You probably won't notice it (maybe you get a compiler warning which annoys you and you add a cast to int) but the result will be surprising. Using operators can be nice sometimes but this is not a good example IMO.

Sven

Actually I would think that operators are used mainly to avoid difficult to spot errors.

In this case it is up to the compiler to check for all the Score variables and divide them in the proper way. If I use a function score_divide() I can miss some place and I can forget some: "int s = some_value / 2;" where some_value is a Score or becomes a Score, according to your example.

If I use a function then "int s = some_value / 2;" will go unoticed and the compiler will do the wrong thing, instead if I use an operator I will be assured that some_value / 2 will be divided correctly as long as _all_ the Score variables have to be divided in that special way (and this is true).

So, although I agree on your starting points, my conclusions seems the opposite IMHO the division operator is useful exactly to avoid some hidden bug because compiler will catch for us all the divions of a Score variable, instead with a function _you_ have to do the job.

As a bottom line I think use of operators enforces type checking, more then ad hoc functions. So it is not only a "nice" thing but more a "safe" thing to do....of course IMHO.

I agree that when using a function like score_divide() instead of a (global) operator, there is some conversion work to do throughout the whole code. But the reason for that has already been created *before* doing that change, by introducing the Score type as enum and defining the special division operator. By doing this, you have changed the semantics of all "int / int"-like expressions in the whole program where the first int was a score - which is good since the idea seems to be an improvement, but which is not visible explicitly. So IMO it would have been a good idea to convert all those expressions *at that time* already.

With the "class Score" proposal in another post of mine, you could as well "easily" make all those special operators and maybe also the make_score(), mg_value(), eg_value() functions members of that class, therefore perhaps avoiding to rename all score division expressions (I haven't tried it).

Sven

[Sven]

It took me about 15 minutes (using the F4 key) to browse in MS Visual Studio through all 263 occurrences of the regular expression

Code: Select all

[^/*]/[^/*]

in SF 1.6 (not (slash or asterisk), followed by slash, followed by not(slash or asterisk)). This finds all single slashes outside of '//', '/*', and '*/' which don't appear as first character on a line. Most matches are within comments, two of them per standard GPL file header. Not many of them are related to scores, so you should be able to find all places very quickly.

Of course, I know this is boring work which one wants to avoid but it should also not be overestimated.

Sven

[mcostalba]

Maybe you can try something like this instead of "enum Score { WHATEVER }", although it is a little bit dangerous, too:

Code: Select all

class Score {
public:
    Score() : v(0) {}
    Score(int val) : v(val) {}
    operator int() { return int(v); }
private:
    int32_t v;
};

The problem is the constructor taking one int as argument. This may lead to some integers being converted to a Score instance by the compiler if he likes to, although I think this is hardly possible without being detected. I have checked that it compiles with MS Visual Studio based on the current public SF 1.6 code but I can't check whether it works, nor do I have a GCC installed right now.

There should be no additional overhead, and the code should be portable, too. Give it a try ...

Sven

Unfortunatly it does NOT compile with gcc and it is also very prone to create hidden copies, for instance something like:

Code: Select all

Score s1 = Score(4);
Score s2 = Score(3 * s1);

will create a Score(4) object that is assigned (copied) to another object s1, then s1 is converted to an integer in the second instruction, is multiplied and then another object Score(3 * s1) is created and then assigned (another copy) to s2.

This is the theory, a compiler _could_ get rid of the hidden copies but you cannot be sure, for instance in a class with non-default c'tor, as is this one, it is very possible that the compiler will not be able to optimize all the above stuff.

In another thread has been written that "it is easier to unknowingly write slow code in c++.", I think _this_ is the case


BTW to use a class to wrap an integer does not smell nice to me...just my personal feeling.

[zamar]

I have tried with this

Code: Select all

/// Score enum keeps a midgame and an endgame value in a single
/// integer (enum), first LSB 16 bits are used to store endgame
/// value, while upper bits are used for midgame value.

enum Score { ENSURE_32_BITS = 1 << 31 };

And indeed the result is different, but still not the same as the intel compiled reference one

Still on my christmas holiday and cannot do the tests myself, but have you tried sth like this:

enum Score { a = INT_MIN, b = INT_MAX };

[Bo Persson]

In particular one of the miscompiled functions (but not the only one) is:

Code: Select all

inline void operator+=(Score& s1, Score s2) { s1 = Score(int(s1) + int(s2)); }

I don't think this is legal code:

enum Score {};

The compiler is allowed to chose the size of an enum variable based on the values it is expected to store. Here you have given it no values!

I can only second that, also from the viewpoint of "clean programming". An enumeration type should have distinct values only, e.g. White, Black, Empty or NoPiece, Pawn, Knight, ..., King. A "score" is something very different unless you enumerate *all* possible values, so it should have a simple definition like "typedef int Score". Attempts to be more clever than that tend to fail at some point, according to my experience at least.

EDIT: With "typedef int Score", user-defined operators for that type like "operator+=" would be redundant.

Sven

yes typedef of int could be a very good idea, but I would like to intercept divsion by an integer value:

Code: Select all

// Division must be handled separately for each term
inline Score operator/(Score s, int i) { return make_score(mg_value(s) / i, eg_value(s) / i); }

I don't think I can do with a typedef .....

No, you can't.

However, if you want a Score type that actually stores two separate values, why not define a class that does just that?

Then you can add exactly those operators you need.

[mcostalba]

I have tried with this

Code: Select all

/// Score enum keeps a midgame and an endgame value in a single
/// integer (enum), first LSB 16 bits are used to store endgame
/// value, while upper bits are used for midgame value.

enum Score { ENSURE_32_BITS = 1 << 31 };

And indeed the result is different, but still not the same as the intel compiled reference one

Still on my christmas holiday and cannot do the tests myself, but have you tried sth like this:

enum Score { a = INT_MIN, b = INT_MAX };

Just tried, it still does not work, although the result is different then with the current version with -O3

I use

./stockfish bench 128 1 12 default depth

to verify node count is the same of the -O1 version, but is not:

Code: Select all

icc 22308138
gcc 22308138 (-O1)
gcc 21577601 (-O3)
gcc 21399015 (-O3) + Joona double enum values

[mcostalba]

However, if you want a Score type that actually stores two separate values, why not define a class that does just that?

Then you can add exactly those operators you need.

I have written something about proness to hidden copies in a post above, I would be very interested in a comment from you on this.

[Bo Persson]

Maybe you can try something like this instead of "enum Score { WHATEVER }", although it is a little bit dangerous, too:

Code: Select all

class Score {
public:
    Score() : v(0) {}
    Score(int val) : v(val) {}
    operator int() { return int(v); }
private:
    int32_t v;
};

The problem is the constructor taking one int as argument. This may lead to some integers being converted to a Score instance by the compiler if he likes to, although I think this is hardly possible without being detected. I have checked that it compiles with MS Visual Studio based on the current public SF 1.6 code but I can't check whether it works, nor do I have a GCC installed right now.

There should be no additional overhead, and the code should be portable, too. Give it a try ...

Sven

Unfortunatly it does NOT compile with gcc and it is also very prone to create hidden copies, for instance something like:

Code: Select all

Score s1 = Score(4);
Score s2 = Score(3 * s1);

will create a Score(4) object that is assigned (copied) to another object s1, then s1 is converted to an integer in the second instruction, is multiplied and then another object Score(3 * s1) is created and then assigned (another copy) to s2.

This is the theory, a compiler _could_ get rid of the hidden copies but you cannot be sure, for instance in a class with non-default c'tor, as is this one, it is very possible that the compiler will not be able to optimize all the above stuff.

Come on, you are copying integers - how expensive is that?

Score s1 = Score(4);

is exactly the same as

int32_t s1 = 4;

The problem we have seen is that the compiler optimzes the code too much, not that it cannot see these simple cases!

Trust your compiler!

In another thread has been written that "it is easier to unknowingly write slow code in c++.", I think _this_ is the case


BTW to use a class to wrap an integer does not smell nice to me...just my personal feeling.

That is perfectly fine, and is done all the time. The compiler sees that the class contains an int member, and works on that.

Some bad code is produced by trying to be too cute, instead of writing simple and straight forward code. The optimizer works best for normal code that the compiler writers see most often, not on tricky code that nobody has ever seen before.

If it is convoluted enough, not even the compiler will understand it.