[PATCH] D115611: [X86][BF16] delete `typedef unsigned short __bfloat16`

Mon Dec 13 18:14:52 PST 2021

pengfei added inline comments.

================
Comment at: clang/lib/Headers/avx512vlbf16intrin.h:416
 ///    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};
----------------
FreddyYe wrote:
> craig.topper wrote:
> > 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.
> 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. 
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`?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115611/new/

https://reviews.llvm.org/D115611