[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

1 ↗

(On Diff #149295)

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

39 ↗

(On Diff #149295)

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

[SjoerdMeijer]

lib/Sema/SemaChecking.cpp

1409 ↗

(On Diff #149295)

Why do we need to remove this?

1462 ↗

(On Diff #149295)

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 ↗

(On Diff #149295)

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

1 ↗

(On Diff #149295)

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?

39 ↗

(On Diff #149295)

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 ↗

(On Diff #149737)

Nit: can you realign this?

2193 ↗

(On Diff #149737)

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 ↗

(On Diff #149737)

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

cfe/trunk/include/clang/Basic/arm_fp16.td

let ArchGuard = "defined(__ARM_FEATURE_FP16_SCALAR_ARITHMETIC) && defined(__aarch64__)" in {
   def SCALAR_FCVTNUH1 : SInst<"vcvtn_u32", "Us", "Sh">;
   def SCALAR_FCVTNUH2 : SInst<"vcvtn_u64", "Os", "Sh">;
   def SCALAR_FCVTPSH  : SInst<"vcvtp_s16", "$s", "Sh">;
   def SCALAR_FCVTPSH1 : SInst<"vcvtp_s32", "Is", "Sh">;
   def SCALAR_FCVTPSH2 : SInst<"vcvtp_s64", "Ls", "Sh">;
   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">;
   def SCALAR_CMGERH   : SInst<"vcge", "bss", "Sh">;
   def SCALAR_CMGEZH   : SInst<"vcgez", "bs", "Sh">;
   def SCALAR_CMGTRH   : SInst<"vcgt", "bss", "Sh">;
   def SCALAR_CMGTZH   : SInst<"vcgtz", "bs", "Sh">;
   def SCALAR_CMLERH   : SInst<"vcle", "bss", "Sh">;

cfe/trunk/lib/Sema/SemaChecking.cpp

#undef GET_NEON_OVERLOAD_CHECK
   }
 
   // For NEON intrinsics which take an immediate value as part of the
   // instruction, range check them here.
   unsigned i = 0, l = 0, u = 0;
   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);
 }
 
 bool Sema::CheckARMBuiltinExclusiveCall(unsigned BuiltinID, CallExpr *TheCall,
                                         unsigned MaxWidth) {
   assert((BuiltinID == ARM::BI__builtin_arm_ldrex ||

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

 // CHECK:  [[TMP2:%.*]] = sext i1 [[TMP1]] to i16
 // CHECK:  ret i16 [[TMP2]]
 uint16_t test_vclth_f16(float16_t a, float16_t b) {
   return vclth_f16(a, b);
 }
 
 // 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
 // CHECK:  [[DIV:%.*]] = fdiv half %a, %b
 // CHECK:  ret half [[DIV]]
 float16_t test_vdivh_f16(float16_t a, float16_t b) {
   return vdivh_f16(a, b);
 }

cfe/trunk/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}}
+}

cfe/trunk/utils/TableGen/NeonEmitter.cpp

if (OI.PtrArgNum >= 0)
       OS << "; PtrArgNum = " << OI.PtrArgNum;
     if (OI.HasConstPtr)
       OS << "; HasConstPtr = true";
     OS << "; break;\n";
   }
   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";
 
   std::set<std::string> Emitted;
 
   for (auto *Def : Defs) {
     if (Def->hasBody())
       continue;
     // Functions with 'a' (the splat code) in the type prototype should not get
     // their own builtin as they use the non-splat variant.
     if (Def->hasSplat())
       continue;
     // Functions which do not have an immediate do not need to have range
     // checking code emitted.
     if (!Def->hasImmediate())
       continue;
     if (Emitted.find(Def->getMangledName()) != Emitted.end())
       continue;
 
     std::string LowerBound, UpperBound;
 
     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
       // instructions have the same bounds and right shifts.
       if (Def->getName().find('r') != std::string::npos ||
           Def->getName().find("cvt") != std::string::npos)
         LowerBound = "1";