Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove one test in the move loop #4815

Closed
wants to merge 1 commit into from

Conversation

snicolet
Copy link
Member

@snicolet snicolet commented Oct 1, 2023

Simplification passed STC test:
https://tests.stockfishchess.org/tests/view/6519fc91cff46e538ee014f6
LLR: 2.94 (-2.94,2.94) <-1.75,0.25>
Total: 191264 W: 48550 L: 48501 D: 94213
Ptnml(0-2): 576, 21529, 51392, 21540, 595

closes https://github.com/snicolet/Stockfish/issues/4815

Non functional change

snicolet added a commit to snicolet/stockfish-pull-requests that referenced this pull request Oct 1, 2023
Gives a very small speed-up (0.3%) on my system

closes official-stockfish#4815

No functional change
@Torom
Copy link
Contributor

Torom commented Oct 2, 2023

ARCH=x86-64-bmi2 clang 16.0.6

Result of  50 runs
==================
base (...es/stockfish) =     911014  +/- 2646
test (...ish.legality) =     907976  +/- 3081
diff                   =      -3038  +/- 4366

speedup        = -0.0033
P(speedup > 0) =  0.0866

CPU: 4 x Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz
Hyperthreading: on

ARCH=x86-64-bmi2 gcc 13.2.1

Result of  50 runs
==================
base (...tockfish_gcc) =     902561  +/- 2624
test (...legality_gcc) =     900882  +/- 3032
diff                   =      -1679  +/- 4673

speedup        = -0.0019
P(speedup > 0) =  0.2410

CPU: 4 x Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz
Hyperthreading: on

@ddobbelaere
Copy link
Contributor

ddobbelaere commented Oct 2, 2023

If anything, I think this patch should be a slowdown, as @Torom also experimentally shows. Note that rootNode is constexpr, hence the compiler knows its value based on the template argument and will never infer extra logic.

But on root nodes, the "legal position check" on master is skipped, as we know the position is legal there anyway (see comments directly above).

Hence, all that this PR does is add an extra unnecessary legality check for root nodes.

I have to acknowledge @peregrineshahin for mentioning this first on Discord.

Simplification passed STC test:
https://tests.stockfishchess.org/tests/view/6519fc91cff46e538ee014f6
LLR: 2.94 (-2.94,2.94) <-1.75,0.25>
Total: 191264 W: 48550 L: 48501 D: 94213
Ptnml(0-2): 576, 21529, 51392, 21540, 595

closes official-stockfish#4815

Non functional change
@vondele vondele added the to be merged Will be merged shortly label Oct 8, 2023
@vondele vondele closed this in 040dfed Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-bench-change to be merged Will be merged shortly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants