Prune illegal moves in qsearch earlier

[Vizvezdenec]

passed STC with elo-gaining bounds
https://tests.stockfishchess.org/tests/view/60f20aefd1189bed71812da0
LLR: 2.94 (-2.94,2.94) <-0.50,2.50>
Total: 61512 W: 4688 L: 4492 D: 52332
Ptnml(0-2): 139, 3730, 22848, 3874, 165
The same version functionally but with moving condition ever earlier passed LTC with simplification bounds.
https://tests.stockfishchess.org/tests/view/60f292cad1189bed71812de9
LLR: 2.98 (-2.94,2.94) <-2.50,0.50>
Total: 60944 W: 1724 L: 1685 D: 57535
Ptnml(0-2): 11, 1556, 27298, 1597, 10
Main idea is that illegal moves influencing search or qsearch obviously can't be any sort of good.
The only reason why initially legality checks for search and qsearch were done after they actually can influence some heuristics is because legality check is expensive computationally.
Eventually in search it was moved to the place where it makes sure that illegal moves can't influence search.
This patch shows that the same can be done for qsearch + it passed STC with elo-gaining bounds + it removes 3 lines of code because you no longer need to increment/decrement movecount on illegal moves.
bench 4709569

[pb00068]

I don't know, I'm not so convinced about this.
First: the simplification is very minimal (just saving one single decrement operation)
Second: while I can agree that in principle illegal moves should'nt influence search,
unless there are some artifacts due to the current implementation I see no need to fix anything.
I someone argues that illegal moves might have influenced search in negative way,
then this patch should actually pass with elo-gaining bounds. Just my 2 cents.
Anyhow no hard feelings and I will accept any decision of the maintainers :-)

[NKONSTANTAKIS]

I was thinking about this and came to the conclusion that its actually quite important.
On one hand we save calculations for later so we do less, but on the other hand we will have cleaner and more appropriate input data. So for me passing simplification bounds is strong, since it faced existent tuning with old input, and future optimization will potentially use this cleaner data.

[Sopel97]

This fixes a bug. The bounds were appropriate.

[Vizvezdenec]

fixes a bug and shrinks code.
Making illegal moves influence search CAN'T ever be good, as simple as that. The only benefit they had is polluting search but making it end up faster.
Seems like this doesn't bring anything now so it's better to not let them pollute search, as simple as that.

[pb00068]

Code robustness is indeed a valid argument.
Anyhow I cannot see a real bug:
even if in some cases we return a value != -VALUE_INFINITE instead of -VALUE_INFINITE ,
it's still clearly below alpha, so alpha-beta search is not broken.
Can one of you gentleman please explain in what the bug consists actually?

[Vizvezdenec]

I think that smth like

      if (futilityValue <= alpha)
      {
          bestValue = std::max(bestValue, futilityValue);
          continue;
      }

can increase bestValue in some cases even if move gets pruned - so basically qsearch can return value that is < alpha but higher than it should be if we wouldn't let illegal moves influence it.
Which then will return into search and start to produce some weird things (for example, wrong probcut cutoffs / stats assignments if value exceeds beta more / break double extensions/ all other things that actually use values outside of (alpha, beta) window with some margin - we have like 4 places in code that do so).
Well, you can also "passively" see that it actually changes a lot since bench change is signifficant.

[Vizvezdenec]

Also another example.
Let's say that illegal move will get generated as 2nd move or earlier and it will get futility pruned.
This is make movecount increase on an illegal move and with this it will break movecount pruning logic in qsearch because it will prune more moves since movecount wouldn't be decremented in current logic since move will never reach legality check.
This isn't fully a "bug" but not that pleasant of a side effect, imho.

[vondele]

I agree that the removal of the moveCount adjustment makes this OK as a simplification.