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

Fix issue https://github.com/official-stockfish/Stockfish/issues/2975 #2976

Closed
wants to merge 1 commit into from

Conversation

mstembera
Copy link
Contributor

@mstembera mstembera commented Aug 10, 2020

This now compiles for me in windows using gcc 7.3, 8.1, and 10.1 and is actually a bit simpler.
After changing the code I could compile but was getting these errors
https://stackoverflow.com/questions/43152633/invalid-register-for-seh-savexmm-in-cygwin
with the older gcc versions but not 10.1.
The -fno-asynchronous-unwind-tables flag fixes those.
If there is a way to check gcc version in the Makefile we could skip this flag for gcc 10 and higher.
(I don't have gcc 9 to test)

Also @vondele asked if we need to adjust the avx512 flags.
We need -mavx512bw and -mavx512f. gcc silently includes -mavx512f when -mavx512bw is specified
but it's probably better to be explicit. (you can test this by adding -mno-avx512f to master and see it fail)
All avx512 implementations however also provide -mavx512cd -mavx512dq -mavx512vl instructions so
we can safely include them too.
https://en.wikichip.org/wiki/x86/avx-512
I measure no speed difference so either way is fine.

No functional change
NNUE: 3994357
bench: 4733874

@vondele
Copy link
Member

vondele commented Aug 10, 2020

I can confirm this builds on linux with gcc 8, 9, and 10.

I would not add the unneeded -mavx512cd -mavx512dq -mavx512vl, just what we need explicitly.

On Linux the flag -fno-asynchronous-unwind-tables is not needed, it is windows specific (see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65782). I don't think I would include that workaround, I don't think it is correct, even if it suppresses the error message (and comes from Agner Fog), it is also fixed on gcc 8, 9, 10 (but latest point releases of those versions).

No functional change
NNUE: 3994357
bench: 4733874
@mstembera
Copy link
Contributor Author

OK Thanks. PR updated.

@gvreuls
Copy link
Contributor

gvreuls commented Aug 10, 2020

@mstembera I bit the bullet and just finished upgrading my development system the next distro release which has GCC 10. (I was under the impression this wasn't going to get fixed, we're asking a lot of people to upgrade their compilers anyway, so why not to GCC 10.)

vondele pushed a commit to vondele/Stockfish that referenced this pull request Aug 11, 2020
avoids an intrinsic that is missing in gcc < 10.

For this target, might trigger another gcc bug on windows that
requires up-to-date gcc 8, 9, or 10, or usage of clang.

Fixes official-stockfish#2975

closes official-stockfish#2976

No functional change
@vondele vondele added the to be merged Will be merged shortly label Aug 11, 2020
@mstembera
Copy link
Contributor Author

@vondele If it's not too late could I please lobby to put the -fno-asynchronous-unwind-tables flag back in? I was working on some other AVX stuff and wanted to test older compilers. I spent a looong time trying to figure out why my new code was failing until I realized it was due to this -fno-asynchronous-unwind-tables flag missing again. I was indifferent earlier but very much an advocate now :)

@vondele
Copy link
Member

vondele commented Aug 11, 2020

Let's make it a separate PR, master is prepared for merging. I would like to understand better, and possibly make it conditional on a windows target? Do we know of side effects of the flag?

@mstembera
Copy link
Contributor Author

NP Thanks. I will get back with a separate PR if it still seems to make sense after I look into it better.

@vondele
Copy link
Member

vondele commented Aug 11, 2020

Yes, my concern is that the code generation is actually wrong in unpatched versions of gcc, and the flag only suppresses the error message which happens to come from unwind-info generation: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65782#c8
But, I don't know these things well enough to judge.

Edit: the fact that it is backported to all open release branches, would support that, this is usually only done for wrong code generation and regressions.

@vondele vondele closed this in f46c730 Aug 11, 2020
joergoster pushed a commit to joergoster/Stockfish-old that referenced this pull request Aug 11, 2020
avoids an intrinsic that is missing in gcc < 10.

For this target, might trigger another gcc bug on windows that
requires up-to-date gcc 8, 9, or 10, or usage of clang.

Fixes official-stockfish/Stockfish#2975

closes official-stockfish/Stockfish#2976

No functional change
lucabrivio pushed a commit to lucabrivio/Stockfish that referenced this pull request Aug 11, 2020
avoids an intrinsic that is missing in gcc < 10.

For this target, might trigger another gcc bug on windows that
requires up-to-date gcc 8, 9, or 10, or usage of clang.

Fixes official-stockfish/Stockfish#2975

closes official-stockfish/Stockfish#2976

No functional change
noobpwnftw pushed a commit to noobpwnftw/Stockfish that referenced this pull request Aug 15, 2020
avoids an intrinsic that is missing in gcc < 10.

For this target, might trigger another gcc bug on windows that
requires up-to-date gcc 8, 9, or 10, or usage of clang.

Fixes official-stockfish#2975

closes official-stockfish#2976

No functional change
Dantist added a commit to Dantist/Stockfish that referenced this pull request Dec 22, 2020
avoids an intrinsic that is missing in gcc < 10.

For this target, might trigger another gcc bug on windows that
requires up-to-date gcc 8, 9, or 10, or usage of clang.

Fixes official-stockfish#2975

closes official-stockfish#2976

No functional change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to be merged Will be merged shortly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants