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

Don't try to use AVX512 without OS support #566

Closed
wants to merge 1 commit into from

Conversation

silby
Copy link

@silby silby commented Feb 23, 2024

AVX512 instructions require OS support as well as CPU support. Bits 5, 6, and 7 of XCR0 should be enabled to run with AVX512 support. This fixes SIGILL on OpenBSD, for example.

OpenBSD ports commit with identical patch

Required alongside the similar fix to simdutf pulled in by #564 for text to work right on OpenBSD when the processor flags AVX512 support.

cbits/measure_off.c Outdated Show resolved Hide resolved
AVX512 instructions require OS support as well as CPU support. Bits 5,
6, and 7 of XCR0 should be enabled to run with AVX512 support. This
fixes SIGILL on OpenBSD, for example.

openbsd/ports@bb29df3
@silby silby requested a review from Bodigrim February 23, 2024 22:14
@Bodigrim
Copy link
Contributor

@silby is it possible to setup a regression CI job? Which OS do not support AVX512?

@silby
Copy link
Author

silby commented Feb 23, 2024

Well the only one I know about is OpenBSD, which is where we discovered the problem. (Whole saga here.) I'm not an expert on really anything involved here I just happened to try to use pandoc on OpenBSD-current on my 11th-gen Intel laptop and banged my head against it until I found enough clues. So I think realistic regression-testing would require specific hardware for the test runner, since it would have to have a processor indicating AVX512 support.

Edit: It looks like to run your Cirrus-CI OpenBSD build with AVX512 you would have to run it on self-managed machines so that you could pick a compute instance type with the required architecture, e.g. Google Compute Engine N2 guarantees at least Cascade Lake. You can't get that granular without provisioning it yourself.

@Bodigrim
Copy link
Contributor

@blackgnezdo could you please advise?

@blackgnezdo
Copy link
Contributor

Since @silby figured this out and tested the fix, I'm pretty comfortable with suggesting this gets merged.

I don't have any insights about where we can host a CI with such special CPU configuration.

@Bodigrim Bodigrim requested a review from Lysxia February 24, 2024 22:47
@Bodigrim
Copy link
Contributor

Could anyone check that the patch does not disable AVX-512 on platforms capable of it? I don't have access to modern Intel CPU at the moment.

@Lysxia
Copy link
Contributor

Lysxia commented Mar 12, 2024

I don't have access to a AVX-512 capable CPU either.

@Bodigrim
Copy link
Contributor

I tried this on macOS x86_64 with AVX-512 instructions and, unfortunately, the patch just disabled their usage. I think this is a peculiarity of macOS (see https://stackoverflow.com/questions/72522885/are-the-xgetbv-and-cpuid-checks-sufficient-to-guarantee-avx2-support and golang/go#43089), but I'm not keen to break macOS for the sake of OpenBSD.

I guess we can guard xgetbv check by #ifndef __APPLE__, but we still need an evidence that xgetbv does not disable AVX-512 on Windows / Linux.

@Bodigrim Bodigrim marked this pull request as draft April 11, 2024 19:57
@Bodigrim
Copy link
Contributor

Converting to draft until someone fixes the behaviour on MacOS and tests it on Windows / Linux machines which have AVX-512.

@falsifian
Copy link

Another problem is that this causes a SIGILL on CPUs that don't have the xgetbv instruction. See here. Checking for the osxsave CPU bit as is done here and avoiding the xgetbv if not set would probably fix it.

(Incidentally, AVX-512 support was just added to OpenBSD-current, so hopefully the original problem no longer affects OpenBSD.)

@Bodigrim
Copy link
Contributor

For the record, an external workaround to disable AVX512 is to pass -D__STDC_NO_ATOMICS__, e. g., via cabal.project.

@silby
Copy link
Author

silby commented Nov 8, 2024

OpenBSD 7.6 shipped with AVX512 support and these patches have been dropped from the lang/ghc port. As I was never an expert to begin with and we don't need this in the port, I'm closing this PR, even though it seems like one could argue that something like this PR is more portable. I've probably forgotten most of what I'm learned while chasing the issue so I'm not really in a position to advise further.

@silby silby closed this Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants