Windows: Meson build fails with nnet_avx2.c compiled without AVX2

[danyeaw]

With opus 1.5.1, building with MSVC on Windows is failing with nnet_avx2.c is being compiled without AVX2 enabled. This is x86_64 architecture computer. The CMake build is working fine.

(tar) Exporting opus
Building project opus (1.5.1)
The Meson build system
Version: 1.3.2
Source dir: C:\gtk-build\build\x64\release\opus
Build dir: C:\gtk-build\build\x64\release\opus\_gvsbuild-meson
Build type: native build
Project name: opus
Project version: 1.5.1
DEPRECATION: None of the values ['gnu99'] are supported by the c compiler.
However, the deprecated gnu99 std currently falls back to c99.
This will be an error in the future.
If the project supports both GNU and MSVC compilers, a value such as
"c_std=gnu11,c11" specifies that GNU is prefered but it can safely fallback to plain c11.
C compiler for the host machine: cl (msvc 19.39.33521 "Microsoft (R) C/C++ Optimizing Compiler Version 19.39.33521 for x64")
C linker for the host machine: link link 14.39.33521.0
Host machine cpu family: x86_64
Host machine cpu: x86_64
Library m found: NO
Checking for function "lrintf" with dependency -lm: YES 
Checking for function "lrint" with dependency -lm: YES 
Checking for function "__malloc_hook" : NO 
Check usable header "stdint.h" : YES 
Checking if "restrict keyword" compiles: YES 
Checking if "C99 variable-size arrays" compiles: NO 
Checking if "alloca (alloca.h)" compiles: NO 
Checking if "alloca (std)" compiles: YES 
Checking if "compiler supports SSE intrinsics" : links: YES 
Checking if "compiler supports SSE2 intrinsics" : links: YES 
Checking if "compiler supports SSE4.1 intrinsics" : links: YES 
Checking if "compiler supports AVX2 intrinsics" : links: YES 
Checking if "Get X86 CPU info via inline assembly" : links: NO 
Checking if "Get X86 CPU info via C method" : links: NO 
Fetching value of define "_MSC_VER" : 1939 
Message: Getting X86 CPU info via __cpuid
Configuring config.h using configuration
Configuring opus.pc using configuration
Program doxygen skipped: feature docs disabled
Build targets in project: 15

opus 1.5.1

  Compiler support
    C99 var arrays                 : NO
    C99 lrintf                     : YES
    Use alloca                     : YES

  Optimizations
    Floating point support         : YES
    Fast float approximations      : NO
    Fixed point debugging          : NO
    Inline assembly optimizations  : none
    External assembly optimizations: none
    Intrinsics optimizations       : SSE, SSE2, SSE4.1, AVX2
    Run-time CPU detection         : SSE, SSE2, SSE4.1, AVX2

  General configuration
    Custom modes                   : NO
    Assertions                     : NO
    Hardening                      : YES
    Fuzzing                        : NO
    Check ASM                      : NO
    API documentation              : NO
    Extra programs                 : YES
    Tests                          : NO

  User defined options
    buildtype                      : debugoptimized
    prefix                         : C:\gtk-build\gtk\x64\release
    docs                           : disabled
    tests                          : disabled

Found ninja-1.11.1 at C:\gtk-build\tools\ninja-1.11.1\ninja.EXE
[143/175] Compiling C object dnn/libdnn_avx2.a.p/x86_nnet_avx2.c.obj
FAILED: dnn/libdnn_avx2.a.p/x86_nnet_avx2.c.obj
"cl" "-Idnn\libdnn_avx2.a.p" "-Idnn" "-I..\dnn" "-I." "-I.." "-Iinclude" "-I..\include" "-Icelt" "-I..\celt" "-Isilk" "-I..\silk" "/MD" "/nologo" "/showInclud
es" "/utf-8" "/W3" "/O2" "/Zi" "-DOPUS_BUILD" "-DHAVE_CONFIG_H" "-D_CRT_SECURE_NO_WARNINGS" "/wd4035" "/wd4715" "/wd4116" "/wd4046" "/wd4068" "/wd4820" "/wd4244" "/wd4255" "/wd4668" "/Fddnn\libdnn_avx2.a.p\x86_nnet_avx2.c.pdb" /Fodnn/libdnn_avx2.a.p/x86_nnet_avx2.c.obj "/c" ../dnn/x86/nnet_avx2.c
../dnn/x86/nnet_avx2.c(35): fatal error C1189: #error:  nnet_avx2.c is being compiled without AVX2 enabled
[147/175] Compiling C object dnn/libdnn_sse2.a.p/x86_nnet_sse2.c.obj
C:\gtk-build\build\x64\release\opus\dnn\vec_avx.h(350): warning C4305: 'function': truncation from 'double' to 'float'
[148/175] Compiling C object dnn/libdnn_sse4_1.a.p/x86_nnet_sse4_1.c.obj
C:\gtk-build\build\x64\release\opus\dnn\vec_avx.h(350): warning C4305: 'function': truncation from 'double' to 'float'
ninja: build stopped: subcommand failed.

[xnorpx]

@jmvalin I think you need an updated version of this: https://gitlab.xiph.org/xiph/opus/-/merge_requests/82

(I am a little overloaded at work but maybe the Meson folks can take a look at it. @tp-m ? )

[jmvalin]

@xnorpx So I had a look at your MR again and I think almost everything is already included except for this part:
https://gitlab.xiph.org/xiph/opus/-/merge_requests/82/diffs#0cc1139e3347f573ae1feee5b73dbc8a8a21fcfa_442_446
Can you explain a bit what that does and whether it's related to the current issue?

[xnorpx]

@jmvalin I take a look tonight and remind myself.

[xnorpx]

@jmvalin the error is caused by nnet_avx2.c is missing build flag: "/arch:AVX2" so that needs to be added in some meson way.

Then it looks like the new options is not printed out

[ 'enable-deep-plc', 'ENABLE_DEEP_PLC' ],
[ 'enable-dred', 'ENABLE_DRED' ],
[ 'enable-osce', 'ENABLE_OSCE' ],

  General configuration
    Custom modes                   : NO
    Assertions                     : NO
    Hardening                      : YES
    Fuzzing                        : NO
    Check ASM                      : NO
    API documentation              : NO
    Extra programs                 : YES
    Tests                          : NO

Finally it looks like dnn dir is added by default not sure if that what you wanted and what is expected?

Is the expectation that

[ 'enable-deep-plc', 'ENABLE_DEEP_PLC' ],
[ 'enable-dred', 'ENABLE_DRED' ],
[ 'enable-osce', 'ENABLE_OSCE' ],

should be off by default and are they independent options?

[jmvalin]

Yeah, I forgot to print the options. The options are meant to be off by default, but not completely independent. Enabling dred or osce should automatically enable Deep PLC.

BTW, I realized that I named the options enable-* when all the other options don't have "enable-" in the name. Think I should change that or will it break too many things at this point?

[xnorpx]

Yeah, I forgot to print the options. The options are meant to be off by default, but not completely independent. Enabling dred or osce should automatically enable Deep PLC.

BTW, I realized that I named the options enable-* when all the other options don't have "enable-" in the name. Think I should change that or will it break too many things at this point?

I would remove enable now from the options. Better break now than never fix it.

@xnorpx So I had a look at your MR again and I think almost everything is already included except for this part: https://gitlab.xiph.org/xiph/opus/-/merge_requests/82/diffs#0cc1139e3347f573ae1feee5b73dbc8a8a21fcfa_442_446 Can you explain a bit what that does and whether it's related to the current issue?

Yes, you need to change it similar to my code change to add /arch:AVX2, you can see that I extended the list a couple of lines above to specify /arch:AVX2 there.

[tp-m]

(I am a little overloaded at work but maybe the Meson folks can take a look at it. @tp-m ? )

I'm traveling this week, but have asked my colleagues to take a look.

We've run into this on the GStreamer CI as well when trying to upgrade to the latest opus.

[xnorpx]

(I am a little overloaded at work but maybe the Meson folks can take a look at it. @tp-m ? )

I'm traveling this week, but have asked my colleagues to take a look.

We've run into this on the GStreamer CI as well when trying to upgrade to the latest opus.

Awesome thanks!

[amyspark]

Hi all, coming here from GStreamer. I've already found and fixed a similar issue in flac, I'll be glad to have a look into it.

[jmvalin]

OK, I think I managed to extract the relevant part of the previous MR. Can you see if this fixes the problem: https://gitlab.xiph.org/xiph/opus/-/commit/601f2722795dcd24a3d3c838e97b8555f84fa85a

[amyspark]

@jmvalin It does fix the issue. Nits:

• I had to disable that new DNN component altogether, I guess it's designed to be used from the tarball only?
• Opus is also falling into the Meson trap of c_std=gnu99 under MSVC, which triggers the standards-non-compliant C89 mode (Meson cannot fall GNU cpp_std flags back to the standard MSVC ones mesonbuild/meson#11641)

For the latter, would it be possible (after fixing this particular issue) to use c_std=c11 and use the _GNU_SOURCE etc. macros, as I did here: https://gitlab.freedesktop.org/gstreamer/meson-ports/x264/-/commit/41af23e8743913ac8cdf1f6d37e5e391ea79a12d ?

[jmvalin]

Regarding the DNN stuff, it should work in all cases, but you need to run the autogen script (or at least the download_model line) to get the model files that aren't in git. Regarding the second issue, I'm not sure I understand but Opus should normally compile with any revision of the standard, including C89.

[jmvalin]

@xnorpx Looking at meson_options.txt, I see there's "boolean" and "feature" options and I haven't quite figured out when one should be used over the other. Any thoughts there (wrt dred, deep-plc, osce)?

[nirbheek]

I would say that you want to use feature options for ... "features", and not for flags

When you want to do automatic detection of when to enable a feature, without the "automagic" problems autotools has. For example, something is platform-specific, or requires external deps, or is an experimental feature. With these, you set the default value of the feature to auto and if the requirements aren't met, the build will just disable that feature and continue.

If you strongly recommend a feature but also want to give people the option to disable it, you can set the default value to enabled.

For configuration options that do not fit any of these, for example things that aren't really a "feature" like picking using fixed point instead of floating-point, fuzzing support, etc, a boolean option is best.

This is all preference, of course. Some people prefer using boolean everywhere except where it's convenient to use feature options: such as with external deps.

The options should definitely not have an enable- prefix though.

[xnorpx]

Regarding the DNN stuff, it should work in all cases, but you need to run the autogen script (or at least the download_model line) to get the model files that aren't in git. Regarding the second issue, I'm not sure I understand but Opus should normally compile with any revision of the standard, including C89.

Is it only for VLA C99 is needed?

[jmvalin]

VLAs are the only C99 features we're using and their use is optional since they're not mandatory in C99. Alternatives include alloca() (defining USE_ALLOCA), or a separate buffer (defining NONTHREADSAFE_PSEUDOSTACK) through that last one is not recommended.

[jmvalin]

@xnorpx and others, can you have a look at https://gitlab.xiph.org/xiph/opus/-/merge_requests/115 and see if that makes sense?

[amyspark]

@jmvalin Looks good to me 👍

[xnorpx]

Yeah, I forgot to print the options. The options are meant to be off by default, but not completely independent. Enabling dred or osce should automatically enable Deep PLC.

Should this be noted in the config as well, i.e. if you enable dred or osce in meson it should enable deep plc as well automagicatlly? (or did I not understand that correctly)

other than that it lgtm.

[amyspark]

Enabling dred or osce should automatically enable Deep PLC.

For those cases I usually prefer something like get_option('deep-plc').enable_auto_if(<check if the other two are enabled>).require(dred or osce, error_message: 'Deep PLC must not be disabled if DRED or OSCE are enabled') which is a bit verboseful, but conveys the chain of requirements.

[jmvalin]

The idea here would be to automatically enable deep-plc when either dred or osce are enabled, not prevent the latter unless deep-plc is enabled. That's the autoconf behaviour.

[xnorpx]

unrelated to this thread but I took a look at CMake and there is no option for DEEP_PLC. Should also write out the abbreviations for dred and osce in the options section.

 ...
 * OPUS_CHECK_ASM, enable bit-exactness checks between optimized and c implementations.
 * OPUS_DNN_FLOAT_DEBUG, Run DNN computations as float for debugging purposes.
 * OPUS_DRED, enable DRED.
 * OPUS_OSCE, enable OSCE.
 * OPUS_FIXED_POINT_DEBUG, debug fixed-point implementation.
 ...

[jmvalin]

Yeah, there's a whole bunch of CMake and meson stuff that needs improvement. I know very little about them so I just did the bare minimum to get things working. Improvements are definitely welcome.