Fix build issue when using clang (#2137)

[anthony-linaro]

Fixes the issue described in #2137 by defining the correct type when using clang-cl (which masquerades as MSVC).

Tested using LLVM 20.1.0's clang-cl, with the following command line:

mkdir build
cd build
cmake -G"Ninja" .. -DCMAKE_C_COMPILER="clang-cl" -DCMAKE_CXX_COMPILER="clang-cl"
cmake --build .
ctest

[anthony-linaro]

FYI @lazka and @kmilos

[kmilos]

Thanks, but I'm not 100% sure this covers #2137 - we're not using clang-cl in MSYS2 CLANGARM64, so _M_ARM64 is not defined AFAIK. llvm-mingw based clang is used instead.

[anthony-linaro]

@kmilos I've just tried it with this line instead, which uses the GNU command line:

cmake -G"Ninja" .. -DCMAKE_C_COMPILER="clang" -DCMAKE_CXX_COMPILER="clang++"

And it still appears to work. Are you able to test to see if it fixes your issue?

[kmilos]

Sorry, that'll have to be @lazka confirming, I have no access...

But generally, I think you'll find that regardless of the frontend command (and architecture), the "official" LLVM Windows binary packages I assume you're using, and the MSYS2 CLANGARM64 (and CLANG64 equally) have different targets defined and are not, in fact, identical/equivalent toolchains: {aarch64,x86_64}-pc-windows-msvc vs {aarch64,x86_64}-w64-windows-gnu?

In other words, both clang and clang-cl targeting *-pc-windows-msvc will in any case emulate MSVC, just that clang-cl supports cl arguments syntax on top vs the GNU style.

[anthony-linaro]

ACK, I'm not able to use that triplet without an msys2 env, so I'll await the response from lazka.

I can't see any other way it would have float16_t defined as int16_t though, unless it was hitting that define statement.

[lazka]

Thanks, but I'm not 100% sure this covers #2137 - we're not using clang-cl in MSYS2 CLANGARM64, so _M_ARM64 is not defined AFAIK. llvm-mingw based clang is used instead.

https://github.com/mingw-w64/mingw-w64/blob/90da6c6535c503159e952dc8b44f8778bed6f622/mingw-w64-headers/crt/_mingw_mac.h#L87

ACK, I'm not able to use that triplet without an msys2 env, so I'll await the response from lazka.

I can't easily test right now, but this looks functionally equivalent to the patch I was using, so lgtm!

[kmilos]

I can't easily test right now, but this looks functionally equivalent to the patch I was using, so lgtm!

Sorry, but I'm still not convinced. This is guarded by _M_ARM64 which should not be defined in CLANGARM64.

Oops, didn't see that link at first, apologies! (So, not defined by the compiler, as expected, but by MinGW headers.)

I'd still prefer to make it explicit like it is in other places then: (defined(__aarch64__) || defined(_M_ARM64))?

[kmilos]

However, we do also have GCC 15 around the corner finally supporting the aarch64-w64-mingw32 target, so the second __clang__ conditional might not be ideal? Don't know (and can't test) if and how float16_t is defined there... Looks like __fp16 is available?

Maybe use #if defined(__GNUC__), or turn the logic around for MSVC specifically #if defined(_MSC_VER) && !defined(__clang__) instead?

[anthony-linaro]

@kmilos I can't easily test the new version of GCC, but I think an inversion may well work here (assuming it does __fp16 as per the docs), so I'll update it to that, and add an explicit check for __aarch64__ too (just in case headers end up differing for some reason in the future and not defining _M_ARM64 anymore)

[kmilos]

Thanks @anthony-linaro

I think you can actually ignore my GCC comment and leave it as __clang__ only: unlike LLVM's, float16_t seems to be defined there in arm_fp16.h (via arm_neon.h), and I guess this was already working on Linux.

[anthony-linaro]

Given that, looking at the code, it's already entirely in an #elif defined(__aarch64__) || defined(_M_ARM64) block, so I'll just clean it up and check for #if !defined(float16_t), and drop the _M_ARM64 bit entirely - the lack of definition for float16_t should suffice

[kmilos]

LGTM