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

[LukeGeeson]

Summary

This fixes the ranges for the vcvth family of FP16 intrinsics in the clang front end. Previously it was accepting incorrect ranges
-Changed builtin range checking in SemaChecking
-added tests SemaCheck changes - included in their own file since no similar one exists
-modified existing tests to reflect new ranges

[SjoerdMeijer]

test/Sema/aarch64-neon-fp16-ranges.c

2

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).

40

why is this is 'a' an int64_t? Should this not be float16_t?

[SjoerdMeijer]

lib/Sema/SemaChecking.cpp

1409

Why do we need to remove this?

1462

And also this one?

[LukeGeeson]

-generates range checks in NEON Emitter rather than hardcoding in SemaChecking.cpp
-uses 'isVCVT_N' to correctly direct the emitter to generate the correct ranges.

[LukeGeeson]

lib/Sema/SemaChecking.cpp

1409

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

test/Sema/aarch64-neon-fp16-ranges.c

2

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?

40

Agreed. Fixed.

[SjoerdMeijer]

I think this looks ok now, just some nits inline.

Can you please upload your diffs with more context next time?

utils/TableGen/NeonEmitter.cpp

2166

Nit: can you realign this?

2193

Nit: for a moment I thought this could match more cases than intended, but we have already checked for isVCVT_N, so should be fine?

2194

Nit: think you're (almost) repeating the comment above, so you can omit this one?

include/clang/Basic/arm_fp16.td

   def SCALAR_FCVTPUH  : SInst<"vcvtp_u16", "bs", "Sh">;
   def SCALAR_FCVTPUH1 : SInst<"vcvtp_u32", "Us", "Sh">;
   def SCALAR_FCVTPUH2 : SInst<"vcvtp_u64", "Os", "Sh">;
-
+  let isVCVT_N = 1 in {
     def SCALAR_SCVTFSHO : SInst<"vcvth_n_f16", "Ysi", "silUsUiUl">;
     def SCALAR_FCVTZSHO : SInst<"vcvt_n_s16", "$si", "Sh">;
     def SCALAR_FCVTZSH1O: SInst<"vcvt_n_s32", "Isi", "Sh">;
     def SCALAR_FCVTZSH2O: SInst<"vcvt_n_s64", "Lsi", "Sh">;
     def SCALAR_FCVTZUHO : SInst<"vcvt_n_u16", "bsi", "Sh">;
     def SCALAR_FCVTZUH1O: SInst<"vcvt_n_u32", "Usi", "Sh">;
     def SCALAR_FCVTZUH2O: SInst<"vcvt_n_u64", "Osi", "Sh">;
-
+  }
   // Comparison
   def SCALAR_CMEQRH   : SInst<"vceq", "bss", "Sh">;
   def SCALAR_CMEQZH   : SInst<"vceqz", "bs", "Sh">;

lib/Sema/SemaChecking.cpp

   switch (BuiltinID) {
   default:
     return false;
   #define GET_NEON_IMMEDIATE_CHECK
   #include "clang/Basic/arm_neon.inc"
   #include "clang/Basic/arm_fp16.inc"
   #undef GET_NEON_IMMEDIATE_CHECK
   }
 
   return SemaBuiltinConstantArgRange(TheCall, i, l, u + l);

test/CodeGen/aarch64-v8.2a-fp16-intrinsics.c

 
 // CHECK-LABEL: test_vcvth_n_f16_s16
 // CHECK: [[SEXT:%.*]] = sext i16 %a to i32
-// CHECK:  [[CVT:%.*]] = call half @llvm.aarch64.neon.vcvtfxs2fp.f16.i32(i32 [[SEXT]], i32 0)
+// CHECK:  [[CVT:%.*]] = call half @llvm.aarch64.neon.vcvtfxs2fp.f16.i32(i32 [[SEXT]], i32 1)
 // CHECK:  ret half [[CVT]]
 float16_t test_vcvth_n_f16_s16(int16_t a) {
-  return vcvth_n_f16_s16(a, 0);
+  return vcvth_n_f16_s16(a, 1);
 }
 
 // CHECK-LABEL: test_vcvth_n_f16_s32
-// CHECK:  [[CVT:%.*]] = call half @llvm.aarch64.neon.vcvtfxs2fp.f16.i32(i32 %a, i32 0)
+// CHECK:  [[CVT:%.*]] = call half @llvm.aarch64.neon.vcvtfxs2fp.f16.i32(i32 %a, i32 1)
 // CHECK:  ret half [[CVT]]
 float16_t test_vcvth_n_f16_s32(int32_t a) {
-  return vcvth_n_f16_s32(a, 0);
+  return vcvth_n_f16_s32(a, 1);
 }
 
 // CHECK-LABEL: test_vcvth_n_f16_s64
-// CHECK:  [[CVT:%.*]] = call half @llvm.aarch64.neon.vcvtfxs2fp.f16.i64(i64 %a, i32 0)
+// CHECK:  [[CVT:%.*]] = call half @llvm.aarch64.neon.vcvtfxs2fp.f16.i64(i64 %a, i32 1)
 // CHECK:  ret half [[CVT]]
 float16_t test_vcvth_n_f16_s64(int64_t a) {
-  return vcvth_n_f16_s64(a, 0);
+  return vcvth_n_f16_s64(a, 1);
 }
 
 // CHECK-LABEL: test_vcvth_n_s16_f16
-// CHECK:  [[CVT:%.*]] = call i32 @llvm.aarch64.neon.vcvtfp2fxs.i32.f16(half %a, i32 0)
+// CHECK:  [[CVT:%.*]] = call i32 @llvm.aarch64.neon.vcvtfp2fxs.i32.f16(half %a, i32 1)
 // CHECK: [[RET:%.*]] = trunc i32 [[CVT]] to i16
 // CHECK: ret i16 [[RET]]
 int16_t test_vcvth_n_s16_f16(float16_t a) {
-  return vcvth_n_s16_f16(a, 0);
+  return vcvth_n_s16_f16(a, 1);
 }
 
 // CHECK-LABEL: test_vcvth_n_s32_f16
-// CHECK:  [[CVT:%.*]] = call i32 @llvm.aarch64.neon.vcvtfp2fxs.i32.f16(half %a, i32 0)
+// CHECK:  [[CVT:%.*]] = call i32 @llvm.aarch64.neon.vcvtfp2fxs.i32.f16(half %a, i32 1)
 // CHECK:  ret i32 [[CVT]]
 int32_t test_vcvth_n_s32_f16(float16_t a) {
-  return vcvth_n_s32_f16(a, 0);
+  return vcvth_n_s32_f16(a, 1);
 }
 
 // CHECK-LABEL: test_vcvth_n_s64_f16
-// CHECK:  [[CVT:%.*]] = call i64 @llvm.aarch64.neon.vcvtfp2fxs.i64.f16(half %a, i32 0)
+// CHECK:  [[CVT:%.*]] = call i64 @llvm.aarch64.neon.vcvtfp2fxs.i64.f16(half %a, i32 1)
 // CHECK:  ret i64 [[CVT]]
 int64_t test_vcvth_n_s64_f16(float16_t a) {
-  return vcvth_n_s64_f16(a, 0);
+  return vcvth_n_s64_f16(a, 1);
 }
 
 // CHECK-LABEL: test_vcvth_n_f16_u16
 // CHECK: [[SEXT:%.*]] = zext i16 %a to i32
-// CHECK:  [[CVT:%.*]] = call half @llvm.aarch64.neon.vcvtfxu2fp.f16.i32(i32 [[SEXT]], i32 0)
+// CHECK:  [[CVT:%.*]] = call half @llvm.aarch64.neon.vcvtfxu2fp.f16.i32(i32 [[SEXT]], i32 1)
 // CHECK:  ret half [[CVT]]
 float16_t test_vcvth_n_f16_u16(int16_t a) {
-  return vcvth_n_f16_u16(a, 0);
+  return vcvth_n_f16_u16(a, 1);
 }
 
 // CHECK-LABEL: test_vcvth_n_f16_u32
-// CHECK:  [[CVT:%.*]] = call half @llvm.aarch64.neon.vcvtfxu2fp.f16.i32(i32 %a, i32 0)
+// CHECK:  [[CVT:%.*]] = call half @llvm.aarch64.neon.vcvtfxu2fp.f16.i32(i32 %a, i32 1)
 // CHECK:  ret half [[CVT]]
 float16_t test_vcvth_n_f16_u32(int32_t a) {
-  return vcvth_n_f16_u32(a, 0);
+  return vcvth_n_f16_u32(a, 1);
 }
 
 // CHECK-LABEL: test_vcvth_n_f16_u64
-// CHECK:  [[CVT:%.*]] = call half @llvm.aarch64.neon.vcvtfxu2fp.f16.i64(i64 %a, i32 0)
+// CHECK:  [[CVT:%.*]] = call half @llvm.aarch64.neon.vcvtfxu2fp.f16.i64(i64 %a, i32 1)
 // CHECK:  ret half [[CVT]]
 float16_t test_vcvth_n_f16_u64(int64_t a) {
-  return vcvth_n_f16_u64(a, 0);
+  return vcvth_n_f16_u64(a, 1);
 }
 
 // CHECK-LABEL: test_vcvth_n_u16_f16
-// CHECK:  [[CVT:%.*]] = call i32 @llvm.aarch64.neon.vcvtfp2fxu.i32.f16(half %a, i32 0)
+// CHECK:  [[CVT:%.*]] = call i32 @llvm.aarch64.neon.vcvtfp2fxu.i32.f16(half %a, i32 1)
 // CHECK: [[RET:%.*]] = trunc i32 [[CVT]] to i16
 // CHECK: ret i16 [[RET]]
 int16_t test_vcvth_n_u16_f16(float16_t a) {
-  return vcvth_n_u16_f16(a, 0);
+  return vcvth_n_u16_f16(a, 1);
 }
 
 // CHECK-LABEL: test_vcvth_n_u32_f16
-// CHECK:  [[CVT:%.*]] = call i32 @llvm.aarch64.neon.vcvtfp2fxu.i32.f16(half %a, i32 0)
+// CHECK:  [[CVT:%.*]] = call i32 @llvm.aarch64.neon.vcvtfp2fxu.i32.f16(half %a, i32 1)
 // CHECK:  ret i32 [[CVT]]
 int32_t test_vcvth_n_u32_f16(float16_t a) {
-  return vcvth_n_u32_f16(a, 0);
+  return vcvth_n_u32_f16(a, 1);
 }
 
 // CHECK-LABEL: test_vcvth_n_u64_f16
-// CHECK:  [[CVT:%.*]] = call i64 @llvm.aarch64.neon.vcvtfp2fxu.i64.f16(half %a, i32 0)
+// CHECK:  [[CVT:%.*]] = call i64 @llvm.aarch64.neon.vcvtfp2fxu.i64.f16(half %a, i32 1)
 // CHECK:  ret i64 [[CVT]]
 int64_t test_vcvth_n_u64_f16(float16_t a) {
-  return vcvth_n_u64_f16(a, 0);
+  return vcvth_n_u64_f16(a, 1);
 }
 
 // CHECK-LABEL: test_vdivh_f16

test/Sema/aarch64-neon-fp16-ranges.c

+// RUN: %clang_cc1 -triple arm64-linux-gnu -fallow-half-arguments-and-returns -target-feature +neon -target-feature +fullfp16 -ffreestanding -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple aarch64-linux-gnu -fallow-half-arguments-and-returns -target-feature +fullfp16 -target-feature +neon -ffreestanding -fsyntax-only -verify %s    
+
+#include <arm_neon.h>
+#include <arm_fp16.h>
+
+void test_vcvt_f16_16(int16_t a){
+  vcvth_n_f16_s16(a, 1);
+  vcvth_n_f16_s16(a, 16);
+  vcvth_n_f16_s16(a, 0);  // expected-error {{argument should be a value from 1 to 16}}
+  vcvth_n_f16_s16(a, 17); // expected-error {{argument should be a value from 1 to 16}}
+
+  vcvth_n_f16_u16(a, 1);
+  vcvth_n_f16_u16(a, 16);
+  vcvth_n_f16_u16(a, 0);  // expected-error {{argument should be a value from 1 to 16}}
+  vcvth_n_f16_u16(a, 17); // expected-error {{argument should be a value from 1 to 16}}
+}
+
+void test_vcvt_f16_32(int32_t a){
+  vcvth_n_f16_u32(a, 1);
+  vcvth_n_f16_u32(a, 16);
+  vcvth_n_f16_u32(a, 0);  // expected-error {{argument should be a value from 1 to 16}}
+  vcvth_n_f16_u32(a, 17); // expected-error {{argument should be a value from 1 to 16}}
+
+  vcvth_n_f16_s32(a, 1);
+  vcvth_n_f16_s32(a, 16);
+  vcvth_n_f16_s32(a, 0);  // expected-error {{argument should be a value from 1 to 16}}
+  vcvth_n_f16_s32(a, 17); // expected-error {{argument should be a value from 1 to 16}}
+}
+
+void test_vcvt_f16_64(int64_t a){
+  vcvth_n_f16_s64(a, 1);
+  vcvth_n_f16_s64(a, 16);
+  vcvth_n_f16_s64(a, 0);  // expected-error {{argument should be a value from 1 to 16}}
+  vcvth_n_f16_s64(a, 17); // expected-error {{argument should be a value from 1 to 16}}
+}
+
+
+void test_vcvt_su_f(float16_t a){
+  vcvth_n_s16_f16(a, 1);
+  vcvth_n_s16_f16(a, 16);
+  vcvth_n_s16_f16(a, 0);  // expected-error {{argument should be a value from 1 to 16}}
+  vcvth_n_s16_f16(a, 17); // expected-error {{argument should be a value from 1 to 16}}
+
+  vcvth_n_s32_f16(a, 1);
+  vcvth_n_s32_f16(a, 16);
+  vcvth_n_s32_f16(a, 0);  // expected-error {{argument should be a value from 1 to 16}}
+  vcvth_n_s32_f16(a, 17); // expected-error {{argument should be a value from 1 to 16}}
+
+  vcvth_n_s64_f16(a, 1);
+  vcvth_n_s64_f16(a, 16);
+  vcvth_n_s64_f16(a, 0);  // expected-error {{argument should be a value from 1 to 16}}
+  vcvth_n_s64_f16(a, 17); // expected-error {{argument should be a value from 1 to 16}}
+
+  vcvth_n_u16_f16(a, 1);
+  vcvth_n_u16_f16(a, 16);
+  vcvth_n_u16_f16(a, 0);  // expected-error {{argument should be a value from 1 to 16}}
+  vcvth_n_u16_f16(a, 17); // expected-error {{argument should be a value from 1 to 16}}
+
+  vcvth_n_u32_f16(a, 1);
+  vcvth_n_u32_f16(a, 16);
+  vcvth_n_u32_f16(a, 0);  // expected-error {{argument should be a value from 1 to 16}}
+  vcvth_n_u32_f16(a, 17); // expected-error {{argument should be a value from 1 to 16}}
+}

utils/TableGen/NeonEmitter.cpp

   OS << "#endif\n\n";
 }
 
-void
-NeonEmitter::genIntrinsicRangeCheckCode(raw_ostream &OS,
+void NeonEmitter::genIntrinsicRangeCheckCode(raw_ostream &OS,
                                         SmallVectorImpl<Intrinsic *> &Defs) {
   OS << "#ifdef GET_NEON_IMMEDIATE_CHECK\n";
 

     Record *R = Def->getRecord();
     if (R->getValueAsBit("isVCVT_N")) {
       // VCVT between floating- and fixed-point values takes an immediate
-      // in the range [1, 32) for f32 or [1, 64) for f64.
+      // in the range [1, 32) for f32 or [1, 64) for f64 or [1, 16) for f16.
       LowerBound = "1";
-      if (Def->getBaseType().getElementSizeInBits() == 32)
+    if (Def->getBaseType().getElementSizeInBits() == 16 ||
+      Def->getName().find('h') != std::string::npos)
+    // VCVTh operating on FP16 intrinsics in range [1, 16)
+    UpperBound = "15";
+    else if (Def->getBaseType().getElementSizeInBits() == 32)
         UpperBound = "31";
-      else
+    else
         UpperBound = "63";
     } else if (R->getValueAsBit("isScalarShift")) {
       // Right shifts have an 'r' in the name, left shifts do not. Convert