craig.topper 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}; ---------------- pengfei wrote: > pengfei wrote: > > 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`? > Sorry, I mean `unsigned short` I'm fine with just changing it to `unsigned short` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115611/new/ https://reviews.llvm.org/D115611 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits