FreddyYe 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};
----------------
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. 


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

Reply via email to