Page MenuHomePhabricator

[clang][AArch64] Correct return type of Neon vqmovun intrinsics
ClosedPublic

Authored by DavidSpickett on Aug 3 2020, 4:03 AM.

Diff Detail

Event Timeline

DavidSpickett created this revision.Aug 3 2020, 4:03 AM
DavidSpickett requested review of this revision.Aug 3 2020, 4:03 AM

Would it be worth looking into improving our general test coverage here? The fact that we don't have any test that verifies the actual signatures of the NEON intrinsics (which could be used to compare against other compilers) seems bad.

I was surprised too. Perhaps there is a way to write the test file such that any type mismatch is a warning and compile with -Werror (maybe a new file, the IR might get messy). I will spend some time looking into this.

Probably the simplest way to verify return types would be using decltype. (static_assert(std::is_same_v<uint32_t, decltype(vqmovund_s64(0))>);.)

For arguments, maybe more complicated. I guess you could write a C++ class with conversion operators for every integer type, mark all of them except one "= delete", and pass it as an argument.

Thanks for the pointer, I found more mistakes this way, e.g.: https://godbolt.org/z/f6n95z
(I don't think I can use <type_traits> in a test but there is clang/test/Headers/x86-64-apple-macosx-types.cpp which uses is_same with it's own implementation so I can use/copy that)

There are ~5 more with incorrect return types so I'll add those to this patch as well as the return type check.
The arguments check is complicated by some intrinsics being macros but we can check the majority, I'll do that in a separate patch.

DavidSpickett planned changes to this revision.Aug 11 2020, 2:14 AM

For intrinsics that are plain functions (not macros) you could just check the signatures using assignments to function pointers, e.g.:

#include <arm_neon.h>
uint16_t (*fp1)(int32_t) = &vqmovuns_s32;
$ clang -target=aarch64-arm-none-eabi -march=armv8-a+simd -c neon.c -fsyntax-only -Werror=incompatible-function-pointer-types
neon.c:2:12: error: incompatible function pointer types initializing 'uint16_t (*)(int32_t)' (aka 'unsigned short (*)(int)') with an expression of type 'int16_t (*)(int32_t)' (aka 'short (*)(int)') [-Werror,-Wincompatible-function-pointer-types]
uint16_t (*fp1)(int32_t) = &vqmovuns_s32;
           ^               ~~~~~~~~~~~~~
1 error generated.
DavidSpickett updated this revision to Diff 292454.EditedSep 17 2020, 3:29 AM

Updated to fix all the vqmovun that have incorrect types.

I'm working on a test specifically for intrinic types, using the function pointer idea (which is how I confirmed these). That will be a separate change.

DavidSpickett edited reviewers, added: efriedma; removed: dnsampaio.Sep 17 2020, 3:30 AM
This revision is now accepted and ready to land.Sep 17 2020, 3:56 PM
This revision was landed with ongoing or failed builds.Sep 21 2020, 1:21 AM
This revision was automatically updated to reflect the committed changes.