[PATCH] D47592: [AArch64] Corrected FP16 Intrinsic range checks in Clang + added Sema tests

[Luke Geeson via Phabricator via cfe-commits]

LukeGeeson marked 2 inline comments as done.
LukeGeeson added inline comments.


================
Comment at: lib/Sema/SemaChecking.cpp:1409
-  switch (BuiltinID) {
-#define GET_NEON_OVERLOAD_CHECK
-#include "clang/Basic/arm_neon.inc"
----------------
SjoerdMeijer wrote:
> Why do we need to remove this?
We don't actually need this one, inspecting include/clang/Basic/arm_fp16.inc after building shows an empty set of #ifdefs  pragmas - ie nothing is generated and hence nothing happens.
Will leave in for continuity, however this may be redundant 


================
Comment at: test/Sema/aarch64-neon-fp16-ranges.c:1
+// RUN: %clang_cc1 -triple arm64-linux-gnu -target-feature +neon -fallow-half-arguments-and-returns -target-feature +fullfp16 -ffreestanding -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -target-feature +neon -fallow-half-arguments-and-returns -target-feature +fullfp16 -ffreestanding -fsyntax-only -verify %s
----------------
SjoerdMeijer wrote:
> Nit: target feature fullfp16 implies ARMv8 FP, so I think you can remove +neon; just a tiny optimisation to make the command line shorter (same below).
That doesn't seem to work, removing neon introduces hundreds of errors around incompatible types (including things this diff doesn't change) which would suggest maybe neon doesn't cover everything?


================
Comment at: test/Sema/aarch64-neon-fp16-ranges.c:39
+
+void test_vcvt_su_f(int64_t a){
+  vcvth_n_s16_f16(a, 1);
----------------
SjoerdMeijer wrote:
> why is this is 'a' an int64_t? Should this not be float16_t?
Agreed. Fixed.


https://reviews.llvm.org/D47592