[Bug target/107748] New: [13 Regression] Isn't _mm_cvtsbh_ss incorrect?

[Bug target/107748] New: [13 Regression] Isn't _mm_cvtsbh_ss incorrect?

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107748

            Bug ID: 107748
           Summary: [13 Regression] Isn't _mm_cvtsbh_ss incorrect?
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jakub at gcc dot gnu.org
  Target Milestone: ---

The implementation:

/* Convert One BF16 Data to One Single Float Data.  */
extern __inline float
__attribute__ ((__gnu_inline__, __always_inline__, __artificial__))
_mm_cvtsbh_ss (__bf16 __A)
{
  union{ float a; unsigned int b;} __tmp;
  __tmp.b = ((unsigned int)(__A)) << 16;
  return __tmp.a;
}

except for the __bfloat16 -> __bf16 change used to be correct in GCC 12,
if one can ignore sNaN (and I presume the builtin is ok with that), then when
__A argument was actually unsigned or signed short, the above did the right
thing.
But now it certainly doesn't, because it converts the floating point BFmode
__A to integer, then shifts the integer 16 bits up and then VIEW_CONVERT_EXPRs
it into float.  So, instead of -ffinite-math-only BF -> SF conversion it
actually
does BF -> SF -> USI conversions, then multiplies by 65536 and then VCE to SF.
So, either it can just do return __A; but that will emit a library call unless
-ffinite-math-only/-ffast-math, or it could be e.g.
  unsigned short int __b;
  unsigned int __c;
  float __ret;
  __builtin_memcpy (&__b, &__A, sizeof (__b));
  __c = __b << 16;
  __builtin_memcpy (&__ret, &__c, sizeof (__ret));
  return __ret;
Note, the a and b identifiers in the union look bad as well, a and b aren't
reserved identifiers...

[Bug target/107748] [13 Regression] Isn't _mm_cvtsbh_ss incorrect?

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107748

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1
                 CC|                            |crazylht at gmail dot com,
                   |                            |hjl.tools at gmail dot com
   Target Milestone|---                         |13.0

[Bug target/107748] [13 Regression] Isn't _mm_cvtsbh_ss incorrect?

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107748

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Discovered as
+FAIL: gcc.target/i386/avx512bf16-cvtsbh2ss-1.c scan-assembler-times sall[
\\\\t]+[^{\\n]*16 1
regression with my
https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606398.html patch,
because previously it emitted just __extendbfsf2 call + insn for SF -> USI
conversion before the shift, but now it emits another shift + SF -> USI
conversion instruction.

[Bug target/107748] [13 Regression] Isn't _mm_cvtsbh_ss incorrect?

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107748

--- Comment #2 from Hongtao.liu <crazylht at gmail dot com> ---
float
_mm_cvtsbh_ss (__bf16 __A)
{
  union{ float sf; __bf16 bf[2];} __tmp;
  __tmp.sf = 0.0f;
  __tmp.bf[1] = __A;
  return __tmp.sf;
}

Looks like gcc can optimize it to

_mm_cvtsbh_ss(bool _Accum):
        movd    %xmm0, %eax
        sall    $16, %eax
        movd    %eax, %xmm0
        ret

[Bug target/107748] [13 Regression] Isn't _mm_cvtsbh_ss incorrect?

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107748

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Hongtao.liu from comment #2)
> float
> _mm_cvtsbh_ss (__bf16 __A)
> {
>   union{ float sf; __bf16 bf[2];} __tmp;
>   __tmp.sf = 0.0f;
>   __tmp.bf[1] = __A;
>   return __tmp.sf;
> }
> 
> Looks like gcc can optimize it to
> 
> _mm_cvtsbh_ss(bool _Accum):
>         movd    %xmm0, %eax
>         sall    $16, %eax
>         movd    %eax, %xmm0
>         ret

That is an option too, but please uglify with __ the sf and bf identifiers
above.
Also, not just for this but more importantly for the __bf16 -> float
conversions
gcc emits for -ffast-math or for cstorebf4 or cbranchcc4, it would be nice if
we optimized those so that if the source and destination are in SSE registers
that we don't convert from SSE to GPR, shift and convert back from GPR to SSE,
while we could do it through some permutation of the SSE register that just
pretends it is a V*HImode and moves the first element to second and zeros the
first (and perhaps all elements above second too, or not, whatever is faster).
Dunno if it could be done as a peephole2, or something different.
Just try:
__attribute__((optimize ("fast-math")))
float foo (__bf16 x) { return x; }
int bar (__bf16 x, __bf16 y) { return x == y; }
void baz (void);
void qux (__bf16 x, __bf16 y) { if (x == y) baz (); }
Oh, and one more thing, for -mavx512bf16 -mavx512vl -ffast-math it would be
nice
to use the AVX512BF16 instruction for float -> __bf16 conversions rather than
library routine.  But that instruction doesn't handle sNaNs properly and
flushes subnormals to 0, so I think we shouldn't do it if HONORS_NANS (BFmode)
or
!flag_unsafe_math_optimizations.

[Bug target/107748] [13 Regression] Isn't _mm_cvtsbh_ss incorrect?

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107748

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 53923
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53923&action=edit
gcc13-pr107748-uglify.patch

Besides the missing uglification in this spot, I found some others (only
checked for . followed by [a-zA-Z] and grepped away what happens usually in
comments, #include/#error lines etc.).  Will test tonight.

[Bug target/107748] [13 Regression] Isn't _mm_cvtsbh_ss incorrect?

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107748

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:ec8ec09f9414be871e322fecf4ebf53e3687bd22

commit r13-4187-gec8ec09f9414be871e322fecf4ebf53e3687bd22
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon Nov 21 10:28:27 2022 +0100

    i386: Uglify some local identifiers in *intrin.h [PR107748]

    While reporting PR107748 (where is a problem with non-uglified names,
    but I've left it out because it needs fixing anyway), I've noticed
    various spots where identifiers in *intrin.h headers weren't uglified.
    The following patch fixed those that are related to unions (I've grepped
    for [a-zA-Z]\.[a-zA-Z] spots).
    The reason we need those to be uglified is the same as why the arguments
    of the inlines are __ prefixed and most of automatic vars in the inlines
    - say a, v or u aren't part of implementation namespace and so users could
     #define u whatever->something
     #include <x86intrin.h>
    and it should still work, as long as u is not e.g. one of the names
    of the functions/macros the header provides (_mm* etc.).

    2022-11-21  Jakub Jelinek  <jakub@redhat.com>

            PR target/107748
            * config/i386/avx512fp16intrin.h (_mm512_castph512_ph128,
            _mm512_castph512_ph256, _mm512_castph128_ph512,
            _mm512_castph256_ph512, _mm512_set1_pch): Uglify names of local
            variables and union members.
            * config/i386/avx512fp16vlintrin.h (_mm256_castph256_ph128,
            _mm256_castph128_ph256, _mm256_set1_pch, _mm_set1_pch): Likewise.
            * config/i386/smmintrin.h (_mm_extract_ps): Likewise.

[Bug target/107748] [13 Regression] Isn't _mm_cvtsbh_ss incorrect?

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107748

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:a6b1f6126de5e45777610699b6d634605c17711c

commit r12-8924-ga6b1f6126de5e45777610699b6d634605c17711c
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon Nov 21 10:28:27 2022 +0100

    i386: Uglify some local identifiers in *intrin.h [PR107748]

    While reporting PR107748 (where is a problem with non-uglified names,
    but I've left it out because it needs fixing anyway), I've noticed
    various spots where identifiers in *intrin.h headers weren't uglified.
    The following patch fixed those that are related to unions (I've grepped
    for [a-zA-Z]\.[a-zA-Z] spots).
    The reason we need those to be uglified is the same as why the arguments
    of the inlines are __ prefixed and most of automatic vars in the inlines
    - say a, v or u aren't part of implementation namespace and so users could
     #define u whatever->something
     #include <x86intrin.h>
    and it should still work, as long as u is not e.g. one of the names
    of the functions/macros the header provides (_mm* etc.).

    2022-11-21  Jakub Jelinek  <jakub@redhat.com>

            PR target/107748
            * config/i386/avx512fp16intrin.h (_mm512_castph512_ph128,
            _mm512_castph512_ph256, _mm512_castph128_ph512,
            _mm512_castph256_ph512, _mm512_set1_pch): Uglify names of local
            variables and union members.
            * config/i386/avx512fp16vlintrin.h (_mm256_castph256_ph128,
            _mm256_castph128_ph256, _mm256_set1_pch, _mm_set1_pch): Likewise.
            * config/i386/smmintrin.h (_mm_extract_ps): Likewise.
            * config/i386/avx512bf16intrin.h (_mm_cvtsbh_ss): Likewise.

    (cherry picked from commit ec8ec09f9414be871e322fecf4ebf53e3687bd22)

[Bug target/107748] [13 Regression] Isn't _mm_cvtsbh_ss incorrect?

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107748

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:961f0e1966549b7ce7c1cbce6a4a91f7062816f0

commit r11-10387-g961f0e1966549b7ce7c1cbce6a4a91f7062816f0
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon Nov 21 10:28:27 2022 +0100

    i386: Uglify some local identifiers in *intrin.h [PR107748]

    While reporting PR107748 (where is a problem with non-uglified names,
    but I've left it out because it needs fixing anyway), I've noticed
    various spots where identifiers in *intrin.h headers weren't uglified.
    The following patch fixed those that are related to unions (I've grepped
    for [a-zA-Z]\.[a-zA-Z] spots).
    The reason we need those to be uglified is the same as why the arguments
    of the inlines are __ prefixed and most of automatic vars in the inlines
    - say a, v or u aren't part of implementation namespace and so users could
     #define u whatever->something
     #include <x86intrin.h>
    and it should still work, as long as u is not e.g. one of the names
    of the functions/macros the header provides (_mm* etc.).

    2022-11-21  Jakub Jelinek  <jakub@redhat.com>

            PR target/107748
            * config/i386/smmintrin.h (_mm_extract_ps): Uglify names of local
            variables and union members.

    (cherry picked from commit ec8ec09f9414be871e322fecf4ebf53e3687bd22)

[Bug target/107748] [13 Regression] Isn't _mm_cvtsbh_ss incorrect?

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107748

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Note, above commits do not fix the reported bug, I've just added identifier
uglification to what I found except _mm_cvtsbh_ss on the trunk, the same
including _mm_cvtsbh_ss on 12 branch (as there _mm_cvtsbh_ss is correct) and
just the _mm_extract_ps case on 11 branch (the only broken one there from what
I could find).

[Bug target/107748] [13 Regression] Isn't _mm_cvtsbh_ss incorrect?

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107748

--- Comment #9 from Hongtao.liu <crazylht at gmail dot com> ---
Since BFmode is most like in xmm register, I'm going to use vector shift
instruction: pslld $16, %xmm0 for extendbfsf2, psrld %16, %xmm0 for truncsfbf2,
It doesn't require any GPR, and no need to use avx512bf16 instruction due to
restriction of flag_unsafe_math_optimizations.

[Bug target/107748] [13 Regression] Isn't _mm_cvtsbh_ss incorrect?

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107748

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by hongtao Liu <liuhongt@gcc.gnu.org>:

https://gcc.gnu.org/g:a1ecc5600464f6a62faab246d522b6328badda90

commit r13-4314-ga1ecc5600464f6a62faab246d522b6328badda90
Author: liuhongt <hongtao.liu@intel.com>
Date:   Wed Nov 23 21:58:09 2022 +0800

    Fix incorrect _mm_cvtsbh_ss.

    After supporting real __bf16, the implementation of _mm_cvtsbh_ss went
    wrong.

    The patch add a builtin to generate pslld for the intrinsic, also
    extendbfsf2 is supported with pslld when !HONOR_NANS (BFmode).

    truncsfbf2 is supported with vcvtneps2bf16 when
    !HONOR_NANS (BFmode) && flag_unsafe_math_optimizations.

    gcc/ChangeLog:

            PR target/107748
            * config/i386/avx512bf16intrin.h (_mm_cvtsbh_ss): Refined.
            * config/i386/i386-builtin-types.def (FLOAT_FTYPE_BFLOAT16):
            New function type.
            * config/i386/i386-builtin.def (BDESC): New builtin.
            * config/i386/i386-expand.cc (ix86_expand_args_builtin):
            Handle the builtin.
            * config/i386/i386.md (extendbfsf2): New expander.
            (extendbfsf2_1): New define_insn.
            (truncsfbf2): Ditto.

    gcc/testsuite/ChangeLog:

            * gcc.target/i386/avx512bf16-cvtsbh2ss-1.c: Scan pslld.
            * gcc.target/i386/extendbfsf.c: New test.

[Bug target/107748] [13 Regression] Isn't _mm_cvtsbh_ss incorrect?

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107748

--- Comment #11 from Hongtao.liu <crazylht at gmail dot com> ---
Fixed in GCC13.

[Bug target/107748] [13 Regression] Isn't _mm_cvtsbh_ss incorrect?

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107748

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
.

[Bug target/107748] [13 Regression] Isn't _mm_cvtsbh_ss incorrect?

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107748

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:02bacbdac4855bdd22f6f1fedd2734ff0afae2db

commit r10-11343-g02bacbdac4855bdd22f6f1fedd2734ff0afae2db
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon Nov 21 10:28:27 2022 +0100

    i386: Uglify some local identifiers in *intrin.h [PR107748]

    While reporting PR107748 (where is a problem with non-uglified names,
    but I've left it out because it needs fixing anyway), I've noticed
    various spots where identifiers in *intrin.h headers weren't uglified.
    The following patch fixed those that are related to unions (I've grepped
    for [a-zA-Z]\.[a-zA-Z] spots).
    The reason we need those to be uglified is the same as why the arguments
    of the inlines are __ prefixed and most of automatic vars in the inlines
    - say a, v or u aren't part of implementation namespace and so users could
     #define u whatever->something
     #include <x86intrin.h>
    and it should still work, as long as u is not e.g. one of the names
    of the functions/macros the header provides (_mm* etc.).

    2022-11-21  Jakub Jelinek  <jakub@redhat.com>

            PR target/107748
            * config/i386/smmintrin.h (_mm_extract_ps): Uglify names of local
            variables and union members.

    (cherry picked from commit ec8ec09f9414be871e322fecf4ebf53e3687bd22)