GCC error - missing cast in MVE arm_biquad_cascade_df1_q15

[kjbracey]

We've had a report that GCC doesn't like the MVE version of arm_biquad_cascade_df1_q15. And its complaint seems valid:

CMSIS-DSP/Source/FilteringFunctions/arm_biquad_cascade_df1_q15.c

Lines 105 to 108 in 302897a

	bCoeffs2 =
	vsetq_lane_s32(vgetq_lane_s32((q31x4_t) bCoeffs0, 3), (q31x4_t) bCoeffs2, 3);
	bCoeffs3 =
	vsetq_lane_s32(vgetq_lane_s32((q31x4_t) bCoeffs1, 3), (q31x4_t) bCoeffs3, 3);

arm_biquad_cascade_df1_q15.c:106:13: error: incompatible types when assigning to type 'q15x8_t' {aka 'int16x8_t'} from type 'int32x4_t'

I'd say you need an explicit (q15x8_t) cast on the output of vsetq_lane_s32.

What's not clear to me is why armclang accepts it, which it does. Indeed, armclang doesn't seem to require the existing (q31x4_t) casts either, at least within that C file.

Pasting the same function into some other C++ code I had handy made armclang produce the equivalent error, unless you have 3 casts. Is it performing implicit conversion in C mode, maybe? Intentionally? Possible armclang bug?

[kjbracey]

While looking around build systems, I've just noticed that GCC's behaviour here is configurable via -flax-vector-conversions.

[christophe0606]

It is advised to use -flax-vector-conversions because there are some cases where the use of the intrinsic is valid but gcc is nevertheless generating a warning.

But, I'll look at above case because I think it needs the cast.

[kjbracey]

Any thoughts on the C vs C++ difference in armclang (if that's what it is)? I don't see such a thing documented. Can you control it?

There were no other reports from our customer of GCC problems without the -flax-vector-conversions, so it's possible that GCC is better than it used to be.

[christophe0606]

I don't know the differences between C and C++ for handling the intrinsics.

So you confirm that for the arm_biquad_cascade_df1_q15, you get cast warnings in C++ with armclang but none from C ?

Can you share the compilation options you used ? I'll try to reproduce and then check with our compiler team.

[kjbracey]

Yes, I've stripped it down, and confirm that it's a C/C++ difference

Given this source:

#include "arm_mve.h"

int16x8_t test(int32x4_t a)
{
    return a;
}

That's accepted without warnings if it's input as C:

armclang --target=arm-arm-none-eabi -mcpu=cortex-m55 -O2 -S vectest.c

But rejected with the type error (not warning) if it comes in as vectest.cpp. Compiler version 6.18.

Conceptually I see no reason for C to be more lax here, and I've not found this documented as being by design. Sure, C++ is more type strict on a few scalar things, like enums and bools, but C has always been just as strict as C++ on aggregates, which these vectors effectively are.

(GCC 11.3 rejects it as either C or C++, unless you have -flax-vector-conversions.)

[christophe0606]

Compiler team tells me that the C and C++ standards are not managing implicit casts in the same way. But otherwise, for this specific example they can't say more.

[kjbracey]

Arm C Language Extensions document just says that such implicit conversions are not portable (for C or C++) - and to use vreinterpretq. (It doesn't actually say explicit conversions via casts are portable.)

So the C compiler is free to give the error, and I think it would be far more helpful if it did. Was trying to check whether this has changed since armcc, but don't have a copy to hand.

[kjbracey]

Minor update, as I found some discussion elsewhere - clang and armclang do also have the -flax-vector-conversions flag, slightly extended to support three options: =none, =int, =all.

Documentation for clang says the default is -flax-vector-conversions=int = -flax-vector-conversions, but as we saw above, the default is actually none for C++.

You can get armclang to reject the above code in C by adding -flax-vector-conversions=none or -fno-lax-vector-conversions. I'll be adding that to my own projects. (I've made too many mistakes from its absence!)

Which in turn means that the change in #65 technically needs to also be applied for armclang. If someone has made vectors strict in the global CMake compile flags (or a future clang has tightened up), it needs to be overridden for the CMSIS-DSP target.

[christophe0606]

@kjbracey Thanks for the feedback !

[christophe0606]

@kjbracey I have added a cmake option to enable / disable lax vector conversions when building for Helium with gcc or ArmClang.
By default it is ON since currently some parts of the library won't build without it.

[kjbracey]

You could scale that back a bit, as I assume you don't actually need "=all", as that's not the default for clang (since clang 10), and it's laxer than gcc, which only permits the integer->integer laxness. So you can set clang to "=integer", or just use the same simple flags as gcc.

"=all" is the clang-9-and-earlier default behaviour and what -flax-vector-conversions used to mean for it, but they changed it to match gcc.

https://releases.llvm.org/10.0.0/tools/clang/docs/ReleaseNotes.html

[christophe0606]

@kjbracey You're right. It build without problems with integer. I have just pushed the change.