lucasart wrote:On the other hand, Stockfish also declares all shared variables as volatile. And I know that Marco is much more knowledgeable than I am in C++, especially when it comes to multi-threading. So I can't help wondering if there isn't indeed a good reason for all this volatile stuff
Many fields of the SplitPoint struct are volatile, but for a reason:
Code: Select all
struct SplitPoint {
...
// Shared data
Mutex mutex;
volatile uint64_t slavesMask;
volatile uint64_t nodes;
volatile Value alpha;
volatile Value bestValue;
volatile Move bestMove;
volatile int moveCount;
volatile bool cutoff;
};
The reason is that these fields are not always accessed under lock protection. For example, in search() you can find several occurrences of:
Code: Select all
if (SpNode)
alpha = splitPoint->alpha;
Without the "volatile" keyword, the compiler might in certain cases be allowed to assume (according to the C++ standard) that splitPoint->alpha cannot change. It could then load this value once from memory and keep it in a register or on the stack. Or the compiler could be allowed to deduce that splitPoint->alpha cannot be different from alpha, so that these lines can be optimised away.
It seems splitPoint->moveCount is always accessed under lock protection (except for an assert), so it seems it could be made non-volatile. It might give the compiler a little bit more freedom when compiling this line:
Code: Select all
moveCount = ++splitPoint->moveCount;
I guess with volatile the compiled code must first read splitPoint->moveCount and increase the value read, and must then write the resulting value to splitPoint->moveCount. It would be illegal to increment splitPoint->moveCount in memory using inc and then read it from memory.
Or do I have it wrong here? Maybe the compiled code must in fact do the following?
1. read splitPoint->moveCount
2. increment the retrieved value
3. write this value to splitPoint->moveCount
4. read this value from splitPoint->moveCount and assign it to moveCount
Does anyone know which of the two (if any of the two) is mandated by the C/C++ abstract machine?
In any case, without volatile anything is allowed as long as the specified result is achieved, and the compiler may assume that no other code or hardware is messing with splitPoint->moveCount (which is in fact a valid assumption due to the locks around this code).
All this means that SF relies on UB. It is not impossible that future compiler optimisations break it in subtle ways. But personally I would advise against fixing this.