[Bug target/85048] [missed optimization] vector conversions

[Bug target/85048] [missed optimization] vector conversions

[mkretz at gcc dot gnu.org]

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

--- Comment #6 from Matthias Kretz (Vir) <mkretz at gcc dot gnu.org> ---
Most of the conversions are optimized perfectly now. Only the following
conversions are still missing for AVX-512:
https://godbolt.org/z/9afWbYod6

#include <cstdint>

template <class T, int N, int Size = N * sizeof(T)>
using V [[gnu::vector_size(Size)]] = T;

template <class From, class To> V<To, 4> cvt4(V<From, 4> x) {
    return V<To, 4>{To(x[0]), To(x[1]), To(x[2]), To(x[3])};
}
template <class From, class To> V<To, 8> cvt8(V<From, 8> x) {
    return V<To, 8>{
        To(x[0]), To(x[1]), To(x[2]), To(x[3]),
        To(x[4]), To(x[5]), To(x[6]), To(x[7])
    };
}
template <class From, class To> V<To, 16> cvt16(V<From, 16> x) {
    return V<To, 16>{
        To(x[0]), To(x[1]), To(x[2]), To(x[3]),
        To(x[4]), To(x[5]), To(x[6]), To(x[7]),
        To(x[8]), To(x[9]), To(x[10]), To(x[11]),
        To(x[12]), To(x[13]), To(x[14]), To(x[15])
    };
}

#define _(name, from, to, size) \
auto name(V<from, size> x) { return cvt##size<from, to>(x); }
// integral -> double
_(vcvtudq2pd, uint32_t, double, 4)
_(vcvtudq2pd, uint32_t, double, 8)

// integral -> float
_(vcvtqq2ps ,  int64_t, float, 16)
_(vcvtuqq2ps, uint64_t, float, 16)

// float -> integral
_(vcvttps2qq, float, int64_t, 16)

_( cvttps2udq, float, uint32_t,  4)
_(vcvttps2udq, float, uint32_t,  8)
_(vcvttps2uqq, float, uint64_t, 16)

// double -> integral
_(vcvttpd2udq, double, uint32_t, 4)

[Bug target/85048] [missed optimization] vector conversions

[crazylht at gmail dot com]

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

Hongtao.liu <crazylht at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |crazylht at gmail dot com

--- Comment #7 from Hongtao.liu <crazylht at gmail dot com> ---
Yes, Looks like the pattern name is misdefined.
it shoud be fixuns_trunc, but we have ufix_trunc.

[Bug target/85048] [missed optimization] vector conversions

[crazylht at gmail dot com]

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

--- Comment #8 from Hongtao.liu <crazylht at gmail dot com> ---
(In reply to Hongtao.liu from comment #7)
> Yes, Looks like the pattern name is misdefined.
> it shoud be fixuns_trunc, but we have ufix_trunc.

No, we have the right name but generate extra instructions for uns.

 8012(define_expand "fixuns_trunc<mode><sseintvecmodelower>2"
 8013  [(match_operand:<sseintvecmode> 0 "register_operand")
 8014   (match_operand:VF1 1 "register_operand")]
 8015  "TARGET_SSE2"
 8016{
 8017  if (<MODE>mode == V16SFmode)
 8018    emit_insn (gen_ufix_truncv16sfv16si2 (operands[0],
 8019                                          operands[1]));
 8020  else
 8021    {
 8022      rtx tmp[3];
 8023      tmp[0] = ix86_expand_adjust_ufix_to_sfix_si (operands[1], &tmp[2]);
 8024      tmp[1] = gen_reg_rtx (<sseintvecmode>mode);
 8025      emit_insn (gen_fix_trunc<mode><sseintvecmodelower>2 (tmp[1],
tmp[0]));
 8026      emit_insn (gen_xor<sseintvecmodelower>3 (operands[0], tmp[1],
tmp[2]));
 8027    }
 8028  DONE;

[Bug target/85048] [missed optimization] vector conversions

[crazylht at gmail dot com]

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

--- Comment #9 from Hongtao.liu <crazylht at gmail dot com> ---
With the patch, we can generate optimized code expect for those 16 {u,}qq
cases, since the ABI doesn't support 1024-bit vector.

1 file changed, 16 insertions(+), 2 deletions(-)
gcc/config/i386/sse.md | 18 ++++++++++++++++--

modified   gcc/config/i386/sse.md
@@ -8014,8 +8014,9 @@ (define_expand "fixuns_trunc<mode><sseintvecmodelower>2"
    (match_operand:VF1 1 "register_operand")]
   "TARGET_SSE2"
 {
-  if (<MODE>mode == V16SFmode)
-    emit_insn (gen_ufix_truncv16sfv16si2 (operands[0],
+  /* AVX512 support vcvttps2udq for all 128/256/512-bit vectors.  */
+  if (<MODE>mode == V16SFmode || TARGET_AVX512VL)
+    emit_insn (gen_ufix_trunc<mode><sseintvecmodelower>2 (operands[0],
                                          operands[1]));
   else
     {
@@ -8413,6 +8414,12 @@ (define_insn "*float<floatunssuffix>v2div2sf2_mask_1"
    (set_attr "prefix" "evex")
    (set_attr "mode" "V4SF")])

+(define_expand "floatuns<si2dfmodelower><mode>2"
+  [(set (match_operand:VF2_512_256VL 0 "register_operand")
+       (unsigned_float:VF2_512_256VL
+         (match_operand:<si2dfmode> 1 "nonimmediate_operand")))]
+   "TARGET_AVX512F")
+
 (define_insn "ufloat<si2dfmodelower><mode>2<mask_name>"
   [(set (match_operand:VF2_512_256VL 0 "register_operand" "=v")
        (unsigned_float:VF2_512_256VL
@@ -8694,6 +8701,13 @@ (define_insn "fix_truncv4dfv4si2<mask_name>"
    (set_attr "prefix" "maybe_evex")
    (set_attr "mode" "OI")])

+
+/* The standard pattern name is fixuns_truncmn2.  */
+(define_expand "fixuns_truncv4dfv4si2"
+  [(set (match_operand:V4SI 0 "register_operand")
+       (unsigned_fix:V4SI (match_operand:V4DF 1 "nonimmediate_operand")))]
+  "TARGET_AVX512VL && TARGET_AVX512F")
+
 (define_insn "ufix_truncv4dfv4si2<mask_name>"
   [(set (match_operand:V4SI 0 "register_operand" "=v")
        (unsigned_fix:V4SI (match_operand:V4DF 1 "nonimmediate_operand"
"vm")))]

[Bug target/85048] [missed optimization] vector conversions

[cvs-commit at gcc dot gnu.org]

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

--- 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:fe42e7fe119159f7443dbe68189e52891dc0148e

commit r13-6951-gfe42e7fe119159f7443dbe68189e52891dc0148e
Author: liuhongt <hongtao.liu@intel.com>
Date:   Thu Mar 30 15:43:25 2023 +0800

    Rename ufix_trunc/ufloat* patterns to fixuns_trunc/floatuns* to align with
standard pattern name.

    There's some typo for the standard pattern name for unsigned_{float,fix},
    it should be floatunsmn2/fixuns_truncmn2, not ufloatmn2/ufix_truncmn2
    in current trunk, the patch fix the typo, also change all though
    ufix_trunc/ufloat patterns.

    Also vcvttps2udq is available under AVX512VL, so it can be generated
    directly instead of being emulated via vcvttps2dq.

    gcc/ChangeLog:

            PR target/85048
            * config/i386/i386-builtin.def (BDESC): Adjust icode name from
            ufloat/ufix to floatuns/fixuns.
            * config/i386/i386-expand.cc
            (ix86_expand_vector_convert_uns_vsivsf): Adjust comments.
            * config/i386/sse.md
            (ufloat<sseintvecmodelower><mode>2<mask_name><round_name>):
            Renamed to ..
           
(<mask_codefor>floatuns<sseintvecmodelower><mode>2<mask_name><round_name>):..
this.
           
(<mask_codefor><avx512>_ufix_notrunc<sf2simodelower><mode><mask_name><round_name>):
            Renamed to ..
           
(<mask_codefor><avx512>_fixuns_notrunc<sf2simodelower><mode><mask_name><round_name>):
            .. this.
            (<fixsuffix>fix_truncv16sfv16si2<mask_name><round_saeonly_name>):
            Renamed to ..
           
(fix<fixunssuffix>_truncv16sfv16si2<mask_name><round_saeonly_name>):.. this.
            (ufloat<si2dfmodelower><mode>2<mask_name>): Renamed to ..
            (floatuns<si2dfmodelower><mode>2<mask_name>): .. this.
            (ufloatv2siv2df2<mask_name>): Renamed to ..
            (<mask_codefor>floatunsv2siv2df2<mask_name>): .. this.
            (ufix_notrunc<mode><si2dfmodelower>2<mask_name><round_name>):
            Renamed to ..
            (fixuns_notrunc<mode><si2dfmodelower>2<mask_name><round_name>):
            .. this.
            (ufix_notruncv2dfv2si2): Renamed to ..
            (fixuns_notruncv2dfv2si2):.. this.
            (ufix_notruncv2dfv2si2_mask): Renamed to ..
            (fixuns_notruncv2dfv2si2_mask): .. this.
            (*ufix_notruncv2dfv2si2_mask_1): Renamed to ..
            (*fixuns_notruncv2dfv2si2_mask_1): .. this.
            (ufix_truncv2dfv2si2): Renamed to ..
            (*fixuns_truncv2dfv2si2): .. this.
            (ufix_truncv2dfv2si2_mask): Renamed to ..
            (fixuns_truncv2dfv2si2_mask): .. this.
            (*ufix_truncv2dfv2si2_mask_1): Renamed to ..
            (*fixuns_truncv2dfv2si2_mask_1): .. this.
            (ufix_truncv4dfv4si2<mask_name>): Renamed to ..
            (fixuns_truncv4dfv4si2<mask_name>): .. this.
            (ufix_notrunc<mode><sseintvecmodelower>2<mask_name><round_name>):
            Renamed to ..
            (fixuns_notrunc<mode><sseintvecmodelower>2<mask_name><round_name>):
            .. this.
            (ufix_trunc<mode><sseintvecmodelower>2<mask_name>): Renamed to ..
            (<mask_codefor>fixuns_trunc<mode><sseintvecmodelower>2<mask_name>):
            .. this.

    gcc/testsuite/ChangeLog:

            * g++.target/i386/pr85048.C: New test.

[Bug target/85048] [missed optimization] vector conversions

[crazylht at gmail dot com]

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

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

[Bug target/85048] [missed optimization] vector conversions

[ubizjak at gmail dot com]

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

--- Comment #12 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Hongtao.liu from comment #9)
> With the patch, we can generate optimized code expect for those 16 {u,}qq
> cases, since the ABI doesn't support 1024-bit vector.

Can't these be vectorized using partial vectors? GCC generates:

_Z9vcvtqq2psDv16_l:
        vmovq   56(%rsp), %xmm0
        vmovq   40(%rsp), %xmm1
        vmovq   88(%rsp), %xmm2
        vmovq   120(%rsp), %xmm3
        vpinsrq $1, 64(%rsp), %xmm0, %xmm0
        vpinsrq $1, 48(%rsp), %xmm1, %xmm1
        vpinsrq $1, 96(%rsp), %xmm2, %xmm2
        vpinsrq $1, 128(%rsp), %xmm3, %xmm3
        vinserti128     $0x1, %xmm0, %ymm1, %ymm1
        vcvtqq2psy      8(%rsp), %xmm0
        vcvtqq2psy      %ymm1, %xmm1
        vinsertf128     $0x1, %xmm1, %ymm0, %ymm0
        vmovq   72(%rsp), %xmm1
        vpinsrq $1, 80(%rsp), %xmm1, %xmm1
        vinserti128     $0x1, %xmm2, %ymm1, %ymm1
        vmovq   104(%rsp), %xmm2
        vcvtqq2psy      %ymm1, %xmm1
        vpinsrq $1, 112(%rsp), %xmm2, %xmm2
        vinserti128     $0x1, %xmm3, %ymm2, %ymm2
        vcvtqq2psy      %ymm2, %xmm2
        vinsertf128     $0x1, %xmm2, %ymm1, %ymm1
        vinsertf64x4    $0x1, %ymm1, %zmm0, %zmm0

where clang manages to vectorize the function to:

  vcvtqq2ps 16(%rbp), %ymm0
  vcvtqq2ps 80(%rbp), %ymm1
  vinsertf64x4 $1, %ymm1, %zmm0, %zmm0

[Bug target/85048] [missed optimization] vector conversions

[mkretz at gcc dot gnu.org]

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

--- Comment #13 from Matthias Kretz (Vir) <mkretz at gcc dot gnu.org> ---
Should I open a new PR for the remaining ((u)int64, 16) <-> (float, 16)
conversions?

https://godbolt.org/z/x3xPMYKj3

Note that __builtin_convertvector produces the code we want.

template <class T, int N, int Size = N * sizeof (T)>
using V [[gnu::vector_size (Size)]] = T;

template <class From, class To>
V<To, 16>
cvt16 (V<From, 16> x)
{
#if BUILTIN
  return __builtin_convertvector (x, V<To, 16>);
#else
  return V<To, 16>{ To (x[0]),  To (x[1]),  To (x[2]),  To (x[3]),
                    To (x[4]),  To (x[5]),  To (x[6]),  To (x[7]),
                    To (x[8]),  To (x[9]),  To (x[10]), To (x[11]),
                    To (x[12]), To (x[13]), To (x[14]), To (x[15]) };
#endif
}

#define _(name, from, to, size)                                               \
  auto name (V<from, size> x) { return cvt##size<from, to> (x); }
// integral -> float
_ (vcvtqq2ps, int64_t, float, 16)
_ (vcvtuqq2ps, uint64_t, float, 16)

// float -> integral
_ (vcvttps2qq, float, int64_t, 16)
_ (vcvttps2uqq, float, uint64_t, 16)

[Bug target/85048] [missed optimization] vector conversions

[liuhongt at gcc dot gnu.org]

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

Hongtao Liu <liuhongt at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |liuhongt at gcc dot gnu.org

--- Comment #14 from Hongtao Liu <liuhongt at gcc dot gnu.org> ---
(In reply to Matthias Kretz (Vir) from comment #13)
> Should I open a new PR for the remaining ((u)int64, 16) <-> (float, 16)
> conversions?
> 
> https://godbolt.org/z/x3xPMYKj3
> 
> Note that __builtin_convertvector produces the code we want.
> 

With -mprefer-vector-width=512, GCC generate produces the same code.
Default tuning for -march=skylake-avx512 is -mprefer-vector-width=256.

[Bug target/85048] [missed optimization] vector conversions

[mkretz at gcc dot gnu.org]

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

--- Comment #15 from Matthias Kretz (Vir) <mkretz at gcc dot gnu.org> ---
So it seems that if at least one of the vector builtins involved in the
expression is 512 bits GCC needs to locally increase prefer-vector-width to
512? Or, more generally:

prefer-vector-width = max(prefer-vector-width, 8 * sizeof(operands)..., 8 *
sizeof(return-value))

The reason to default to 256 bits is to avoid zmm register usage altogether
(clock-down). But if the surrounding code already uses zmm registers that
motivation is moot.

Also, I think this shouldn't be considered auto-vectorization but rather
pattern recognition (recognizing a __builtin_convertvector).

[Bug target/85048] [missed optimization] vector conversions

[liuhongt at gcc dot gnu.org]

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

--- Comment #16 from Hongtao Liu <liuhongt at gcc dot gnu.org> ---
(In reply to Matthias Kretz (Vir) from comment #15)
> So it seems that if at least one of the vector builtins involved in the
> expression is 512 bits GCC needs to locally increase prefer-vector-width to
> 512? Or, more generally:
> 
> prefer-vector-width = max(prefer-vector-width, 8 * sizeof(operands)..., 8 *
> sizeof(return-value))
> 
> The reason to default to 256 bits is to avoid zmm register usage altogether
> (clock-down). But if the surrounding code already uses zmm registers that
> motivation is moot.
> 
> Also, I think this shouldn't be considered auto-vectorization but rather
> pattern recognition (recognizing a __builtin_convertvector).

The related question is "should GCC set prefer-vector-width=512" when 512-bit
intrinsics is used. There may be a situation where users don't want compiler to
generate zmm except for those 512-bit intrinsics in their program, i.e the hot
loop is written with 512-bit intrinsics for performance purpose, but for other
places, better no zmm usage.