Skip to content

Dynamic AVX detection is broken for MSVC #15292

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

Closed
cmb69 opened this issue Aug 8, 2024 · 1 comment
Closed

Dynamic AVX detection is broken for MSVC #15292

cmb69 opened this issue Aug 8, 2024 · 1 comment

Comments

@cmb69
Copy link
Member

cmb69 commented Aug 8, 2024

Description

4e30ab3 made a general important improvement for proper AVX detection, but unfortunately guarded the relevant code with

#if defined(__i386__) || defined(__x86_64__)

Neither of these macros are pre-defined for MSVC, what results in the following function definition:

php-src/Zend/zend_cpuinfo.c

Lines 109 to 111 in c68b43c

static bool is_avx_supported(void) {
return 0;
}

That means that even though AVX and even AVX2 are supported by the processor, no support is flagged, and the optimized implementations are never called.

If I change the guard code to

#if defined(_WIN32) || defined(_WIN64)

AVX detections works on my machine (x64, have not tested x86 yet), but I'm not sure whether these would be the appropriate macros, or whether ARM needs special treatment. Maybe someone else knows, or could test it (I don't have an ARM available).

In my opinion, we need to fix this for master and may consider to target a lower branch (maybe backport only after a while, when PHP 8.4 has been sufficiently tested).

PHP Version

PHP 7.4+

Operating System

Windows

@nielsdos
Copy link
Member

nielsdos commented Aug 8, 2024

Adding additional or-checks for _M_IX86 or _M_AMD64 should do the trick.
See https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170

nielsdos added a commit to nielsdos/php-src that referenced this issue Aug 8, 2024
See https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170
For x64, either _M_X64 or _M_AMD64 would work but I'm going with what's
already used in php-src.
@nielsdos nielsdos linked a pull request Aug 8, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants