[X86][BF16] delete `typedef unsigned short __bfloat16`

[FreddyYe]

Summary

The name __bfloat may mislead its real type is bfloat16, but in
fact it's not.

[craig.topper]

clang/lib/Headers/avx512vlbf16intrin.h

416

I'm not sure if this change is a good idea this late. Users could have been dependent on it being an unsigned value before. I believe this changes the behavior of this code

int result = _mm_cvtness_sbh(X)

Previously it would have zero extended, but now it will sign extend.

[FreddyYe]

clang/lib/Headers/avx512vlbf16intrin.h

416

Yes, this should be a huge concern.

Notice that intrinsic update has just documented these two intrinsics on 12/7/2021. So maybe we still have change to change it? And it's more theory right to do sign extension from a bfloat16 to int32.

[pengfei]

clang/lib/Headers/avx512vlbf16intrin.h

416

I think this is the problem that we choose integer to represent BF16. Neither zero extend nor sign extend makes sense to a floating type. But considering the MSB of floating point is sign bit. Sign extend should be better in theory.
Maybe it's a good approach to use __bf16, but it is supported only by Clang. We can't use it for intrinsics.
Anyway, I'm fine with keeping zero extend here. @craig.topper , do you think it's acceptable to you if we just change __bfloat16 to short?

[pengfei]

clang/lib/Headers/avx512vlbf16intrin.h

416

Sorry, I mean unsigned short

[craig.topper]

clang/lib/Headers/avx512vlbf16intrin.h

416

I'm fine with just changing it to unsigned short

[FreddyYe]

changed into unsigned short

[FreddyYe]

clang-format

[skan]

LGTM

[pengfei]

I would suggest we drop the change. Sorry for my fickleness :(

clang/lib/Headers/avx512bf16intrin.h

32

Sorry, when I reviewed the doxygen, I found a new problem. If we want to use unsigned short, we may need to change here too. But unsigned short is not clear to user since they actually want to convert a bfloat type instead of integer.
On the other hand, the double underscore naming conversion is reserved for compiler use and we have already used lots of these terminologies for X86 intrinsics. A much similar one is __mask16. So I think using __bfloat16 here is consistent with the existing types we have defined.

[FreddyYe]

agree with @pengfei . sorry for noise.

clang/lib/CodeGen/CGBuiltin.cpp

static Value *EmitX86CvtF16ToFloatExpr(CodeGenFunction &CGF,
   return Res;
 }
 
 // Convert a BF16 to a float.
 static Value *EmitX86CvtBF16ToFloatExpr(CodeGenFunction &CGF,
                                         const CallExpr *E,
                                         ArrayRef<Value *> Ops) {
   llvm::Type *Int32Ty = CGF.Builder.getInt32Ty();
-  Value *ZeroExt = CGF.Builder.CreateZExt(Ops[0], Int32Ty);
-  Value *Shl = CGF.Builder.CreateShl(ZeroExt, 16);
+  Value *SignExt = CGF.Builder.CreateSExt(Ops[0], Int32Ty);
+  Value *Shl = CGF.Builder.CreateShl(SignExt, 16);
   llvm::Type *ResultType = CGF.ConvertType(E->getType());
   Value *BitCast = CGF.Builder.CreateBitCast(Shl, ResultType);
   return BitCast;
 }
 
 Value *CodeGenFunction::EmitX86CpuIs(StringRef CPUStr) {
 
   llvm::Type *Int32Ty = Builder.getInt32Ty();

clang/lib/Headers/avx512bf16intrin.h

 #error "Never use <avx512bf16intrin.h> directly; include <immintrin.h> instead."
 #endif
 
 #ifndef __AVX512BF16INTRIN_H
 #define __AVX512BF16INTRIN_H
 
 typedef short __m512bh __attribute__((__vector_size__(64), __aligned__(64)));
 typedef short __m256bh __attribute__((__vector_size__(32), __aligned__(32)));
-typedef unsigned short __bfloat16;
 
 #define __DEFAULT_FN_ATTRS512 \
   __attribute__((__always_inline__, __nodebug__, __target__("avx512bf16"), \
                  __min_vector_width__(512)))
 #define __DEFAULT_FN_ATTRS                                                     \
   __attribute__((__always_inline__, __nodebug__, __target__("avx512bf16")))
 
 /// Convert One BF16 Data to One Single Float Data.
 ///
 /// \headerfile <x86intrin.h>
 ///
 /// This intrinsic does not correspond to a specific instruction.
 ///
 /// \param __A
 ///    A bfloat data.
 /// \returns A float data whose sign field and exponent field keep unchanged,
 ///    and fraction field is extended to 23 bits.
-static __inline__ float __DEFAULT_FN_ATTRS _mm_cvtsbh_ss(__bfloat16 __A) {
+static __inline__ float __DEFAULT_FN_ATTRS _mm_cvtsbh_ss(short __A) {
   return __builtin_ia32_cvtsbf162ss_32(__A);
 }
 
 /// Convert Two Packed Single Data to One Packed BF16 Data.
 ///
 /// \headerfile <x86intrin.h>
 ///
 /// This intrinsic corresponds to the <c> VCVTNE2PS2BF16 </c> instructions.

clang/lib/Headers/avx512vlbf16intrin.h

 /// \headerfile <x86intrin.h>
 ///
 /// This intrinsic corresponds to the <c> VCVTNEPS2BF16 </c> instructions.
 ///
 /// \param __A
 ///    A float data.
 /// \returns A bf16 data whose sign field and exponent field keep unchanged,
 ///    and fraction field is truncated to 7 bits.
-static __inline__ __bfloat16 __DEFAULT_FN_ATTRS128 _mm_cvtness_sbh(float __A) {
+static __inline__ short __DEFAULT_FN_ATTRS128 _mm_cvtness_sbh(float __A) {
   __v4sf __V = {__A, 0, 0, 0};
   __v8hi __R = __builtin_ia32_cvtneps2bf16_128_mask(
       (__v4sf)__V, (__v8hi)_mm_undefined_si128(), (__mmask8)-1);
   return __R[0];
 }
 
 /// Convert Packed BF16 Data to Packed float Data.
 ///

clang/test/CodeGen/X86/avx512bf16-builtins.c

 //  RUN: %clang_cc1 -flax-vector-conversions=none -ffreestanding %s -triple=x86_64-apple-darwin \
 //  RUN:            -target-feature +avx512bf16 -emit-llvm -o - -Wall -Werror \
 //  RUN:            | FileCheck %s
 
 #include <immintrin.h>
 
-float test_mm_cvtsbh_ss(__bfloat16 A) {
+float test_mm_cvtsbh_ss(short A) {
   // CHECK-LABEL: @test_mm_cvtsbh_ss
-  // CHECK: zext i16 %{{.*}} to i32
+  // CHECK: sext i16 %{{.*}} to i32
   // CHECK: shl i32 %{{.*}}, 16
   // CHECK: bitcast i32 %{{.*}} to float
   // CHECK: ret float %{{.*}}
   return _mm_cvtsbh_ss(A);
 }
 
 __m512bh test_mm512_cvtne2ps_pbh(__m512 A, __m512 B) {
   // CHECK-LABEL: @test_mm512_cvtne2ps_pbh

clang/test/CodeGen/X86/avx512vlbf16-builtins.c

 __m256 test_mm256_mask_dpbf16_ps(__m256 D, __m256bh A, __m256bh B, __mmask8 U) {
   // CHECK-LABEL: @test_mm256_mask_dpbf16_ps
   // CHECK: @llvm.x86.avx512bf16.dpbf16ps.256
   // CHECK: select <8 x i1> %{{.*}}, <8 x float> %{{.*}}, <8 x float> %{{.*}}
   // CHECK: ret <8 x float> %{{.*}}
   return _mm256_mask_dpbf16_ps(D, U, A, B);
 }
 
-__bfloat16 test_mm_cvtness_sbh(float A) {
+short test_mm_cvtness_sbh(float A) {
   // CHECK-LABEL: @test_mm_cvtness_sbh
   // CHECK: @llvm.x86.avx512bf16.mask.cvtneps2bf16.128
   // CHECK: ret i16 %{{.*}}
   return _mm_cvtness_sbh(A);
 }
 
 __m128 test_mm_cvtpbh_ps(__m128bh A) {
   // CHECK-LABEL: @test_mm_cvtpbh_ps