The unstable-simd module doesn't check that neon code can be compiled

[tanuk]

Describe the bug After switching from Autotools to Meson in the PulseAudio recipe in OpenEmbedded, there have been two reports of compilation failures related to Neon optimized code getting enabled when it shouldn't be. I had a quick look at the unstable-simd module implementation, and it looks like it only tests that the compiler supports the -mfpu=neon argument. However, it's possible that the compiler accepts the argument but still can't compile neon code.

The first failure was with clang when compiling for x86. The failure happened when using the -Qunused-arguments argument, without that argument the build succeeded. I suppose -Qunused-arguments had the effect of ignoring -mfpu=neon even on x86, so the check that Meson did passed and the neon code was incorrectly enabled. Link to discussion: https://lists.openembedded.org/g/openembedded-core/topic/79044755#145899

The second failure was on armv5 with soft-float ABI. The arm_neon.h header doesn't support the soft-float ABI, causing failure when including the header. Link to discussion: https://lists.openembedded.org/g/openembedded-core/topic/79283711#146289

To me it seems that the unstable-simd module should try compiling some neon code (or at least try including arm_neon.h) before concluding that neon is supported. If it did that, both of these failures would have been avoided.

To Reproduce Sorry, I don't have simple test cases (at least yet). In theory just trying to compile PulseAudio with clang on x86 should trigger the problem when using the -Qunused-arguments compiler argument (I haven't tried myself yet). PulseAudio's code can be found here: https://gitlab.freedesktop.org/pulseaudio/pulseaudio and the meson.build file using the unstable-simd module is here: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/blob/master/src/pulsecore/meson.build

Expected behavior The unstable-simd module should detect when neon code can't be compiled.

system parameters

• Is this a cross build or just a plain native build (for the same computer)?
  • These are cross builds.
• what operating system (e.g. MacOS Catalina, Windows 10, CentOS 8.0, Ubuntu 18.04, etc.)
  • I don't know the operating system in the armv5 case, but in the clang case the build machine seems to have been Ubuntu 18.04. However, that has little relevance, because most of the build environment comes from OpenEmbedded (the master branch in these cases).
• what Python version are you using e.g. 3.8.0
  • OpenEmbedded seems to have Python 3.9.1 at the moment.
• what meson --version
  • OpenEmbedded seems to have Meson 0.56.0 at the moment.
• what ninja --version if it's a Ninja build
  • OpenEmbedded seems to have Ninja 1.10.2 at the moment.

[eli-schwartz]

At first glance, this clang flag seems like it's the kind of thing that should break the functionality of meson.get_compiler('c').has_argument().

Why are people passing invalid arguments to clang, and then passing more arguments that cause invalid arguments to not produce errors? Why not just refrain from passing invalid arguments -- or, use the dedicated meson compiler object methods for detecting which arguments you'd like to use, are supported, and only using those?

[tanuk]

I don't know why -Qunused-arguments is used, maybe @kraj can comment, he's the one who ran into this issue.

The -Qunused-arguments problem can be worked around in OpenEmbedded, but the soft-float issue is somewhat more problematic.

Fixing the soft-float issue in Meson will probably fix the -Qunused-arguments issue as well.

[kraj]

@eli-schwartz I agree with you on packages doing the right thing and perhaps be aware of the portability across compilers but that is not the case yet. this is used since Clang can come up with additional warnings about unused arguments which maybe is gcc specific in several packages this fails the build with clang so using -Qunused-arguments gets you through this. Regardless if this is will be good to rely on feature tests instead of compiler diagnostic outputs to consider a configure test as pass or fail perhaps.

[eli-schwartz]

This is a reasonable argument, but it is unstable for a reason. :p

The current check is indeed very primitive: https://github.com/mesonbuild/meson/blob/f9dd75f213b1c3a7ab397133b6a157ddba511d90/mesonbuild/modules/unstable_simd.py#L64-L72

Any interest in implementing an additional compiler check? Is this needed for any SIMD intrinsics other than neon soft-float?

[armcc]

Just for reference, here's the PulseAudio autotools test for NEON support:

https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/blob/b6396dbe9c97be844553dd748e7cf9c3efdc0086/configure.ac#L356

Auto detection works by prepending -mfpu=neon to CFLAGS and then verifying that compilation with #include succeeds (it also supports --enable-neon-opt / --disable-neon-opt configure options to disable the auto detection).

If NEON support is enabled, -mfpu=neon is stored in NEON_CFLAGS which is then later applied when compiling the few specific .c sources which include NEON based inline assembler. It looks like the PulseAudio Meson build using unstable-simd may be missing this second part?

[tanuk]

If NEON support is enabled, -mfpu=neon is stored in NEON_CFLAGS which is then later applied when compiling the few specific .c sources which include NEON based inline assembler. It looks like the PulseAudio Meson build using unstable-simd may be missing this second part?

Do you mean missing -mfpu=neon in CFLAGS? I'm pretty sure the simd module adds -mfpu=neon to the CFLAGS of libpulsecore_simd_lib, otherwise compiling any neon code would always fail.

[tanuk]

Any interest in implementing an additional compiler check?

I may attempt to implement that if nobody else does, but I can't promise any timeline for that.

Is this needed for any SIMD intrinsics other than neon soft-float?

I haven't so far seen issues with mmx or sse (which are the two other SIMD instruction sets that PulseAudio supports), although I imagine mmx/sse might get incorrectly enabled on arm if compiling with clang and -Qunused-arguments.

[armcc]

Do you mean missing -mfpu=neon in CFLAGS? I'm pretty sure the simd module adds -mfpu=neon to the CFLAGS of libpulsecore_simd_lib, otherwise compiling any neon code would always fail.

I'm not sure. If -mfpu=neon were being added automatically then I don't think we'd see the issue currently being reported in OE?

In general, I think there are three basic cases:

1) The compiler supports NEON inline asm without any additional CFLAGS (e.g. because of global -mcpu=XXX or -mfpu=XXX options already on the command line). 2) The compiler can support NEON inline asm if -mfpu=neon is appended to the existing command line options. 3) The compiler can't support NEON inline asm at all (e.g. when building for something other than ARM).

The current issue with OE seems to be case 2. The target is ARMv5 Soft Float (no NEON support).

For case 2 there are two possible approaches: either completely disable NEON support at build time or add -mfpu=neon for the specific sources which need it and then rely on the application to detect NEON support at runtime only call the NEON based functions when running on targets which can support it.

[tanuk]

I'm not sure. If -mfpu=neon were being added automatically then I don't think we'd see the issue currently being reported in OE?

The compiler command line can be seen in here for the x86 failure: https://errors.yoctoproject.org/Errors/Details/539474/

Even though the build is for x86, the command line includes -mfpu=neon when compiling remap_neon.c, which I think is a pretty clear indication that Meson adds -mfpu=neon to CFLAGS when building neon code.

As far as I can tell, the armv5 soft float issue doesn't suggest that -mfpu=neon is missing from the command line, I believe the problem is that -mfloat-abi is set to "soft" rather than "softfp" or "hard". arm_neon.h requires the ABI to be "softfp" or "hard".

[armcc]

As far as I can tell, the armv5 soft float issue doesn't suggest that -mfpu=neon is missing from the command line, I believe the problem is that -mfloat-abi is set to "soft" rather than "softfp" or "hard". arm_neon.h requires the ABI to be "softfp" or "hard".

OK. That makes sense. If the target is soft float (ie -mfloat-abi=soft) then it doesn't make much sense to have a run time check for NEON, so a build time check as already proposed sounds fine.

[jpakkane]

One of the problems here is the -Qunused-arguments arg. Where is that coming from? Who is injecting it and why? This seems like a dangerous thing to be using in general as it breaks things like checks.

We do filter out some compiler flags that break things when compiling test executables. This is one that could be added to that set. It might fix other such issues in the future (assuming it works and does the right thing here).

[kraj]

One of the problems here is the -Qunused-arguments arg. Where is that coming from? Who is injecting it and why? This seems like a dangerous thing to be using in general as it breaks things like checks.

We do filter out some compiler flags that break things when compiling test executables. This is one that could be added to that set. It might fix other such issues in the future (assuming it works and does the right thing here).

The use of -Qunused-arguments is already discussed in this thread earlier. The question is should build system checks for processor features depend on guessing from compiler warnings that are non-specific or should there be a better check perhaps using builtins etc. to ensure the feature is supported. if you think meson should disregard -Qunused-arguments then do so explicitly in the checks where this is expected even though I would find that inferior solution it perhaps is better than the status quo