Re: [PATCH 1/2] i386: Enable AVX512 memory broadcast for FP mul

[Uros Bizjak <ubizjak at gmail dot com>]

On Sat, Oct 20, 2018 at 11:47 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On 10/20/18, Uros Bizjak <ubizjak@gmail.com> wrote:
> > On Fri, Oct 19, 2018 at 11:08 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>
> >> Many AVX512 vector operations can broadcast from a scalar memory source.
> >> This patch enables memory broadcast for FP mul operations.
> >>
> >> gcc/
> >>
> >>         PR target/72782
> >>         * config/i386/sse.md (*mul<mode>3<mask_name>_bcst_1): New.
> >>         (*mul<mode>3<mask_name>_bcst_2): Likewise.
> >>
> >> gcc/testsuite/
> >>
> >>         PR target/72782
> >>         * gcc.target/i386/avx512f-mul-df-zmm-1.c: New test.
> >>         * gcc.target/i386/avx512f-mul-sf-zmm-1.c: Likewise.
> >>         * gcc.target/i386/avx512f-mul-sf-zmm-2.c: Likewise.
> >>         * gcc.target/i386/avx512f-mul-sf-zmm-3.c: Likewise.
> >>         * gcc.target/i386/avx512f-mul-sf-zmm-4.c: Likewise.
> >>         * gcc.target/i386/avx512f-mul-sf-zmm-5.c: Likewise.
> >>         * gcc.target/i386/avx512f-mul-sf-zmm-6.c: Likewise.
> >>         * gcc.target/i386/avx512vl-mul-sf-xmm-1.c: Likewise.
> >>         * gcc.target/i386/avx512vl-mul-sf-ymm-1.c: Likewise.
> >> ---
> >>  gcc/config/i386/sse.md                        | 24 +++++++++++++++++++
> >>  .../gcc.target/i386/avx512f-mul-df-zmm-1.c    | 12 ++++++++++
> >>  .../gcc.target/i386/avx512f-mul-sf-zmm-1.c    | 12 ++++++++++
> >>  .../gcc.target/i386/avx512f-mul-sf-zmm-2.c    | 12 ++++++++++
> >>  .../gcc.target/i386/avx512f-mul-sf-zmm-3.c    | 12 ++++++++++
> >>  .../gcc.target/i386/avx512f-mul-sf-zmm-4.c    | 12 ++++++++++
> >>  .../gcc.target/i386/avx512f-mul-sf-zmm-5.c    | 12 ++++++++++
> >>  .../gcc.target/i386/avx512f-mul-sf-zmm-6.c    | 12 ++++++++++
> >>  .../gcc.target/i386/avx512vl-mul-sf-xmm-1.c   | 12 ++++++++++
> >>  .../gcc.target/i386/avx512vl-mul-sf-ymm-1.c   | 12 ++++++++++
> >>  10 files changed, 132 insertions(+)
> >>  create mode 100644 gcc/testsuite/gcc.target/i386/avx512f-mul-df-zmm-1.c
> >>  create mode 100644 gcc/testsuite/gcc.target/i386/avx512f-mul-sf-zmm-1.c
> >>  create mode 100644 gcc/testsuite/gcc.target/i386/avx512f-mul-sf-zmm-2.c
> >>  create mode 100644 gcc/testsuite/gcc.target/i386/avx512f-mul-sf-zmm-3.c
> >>  create mode 100644 gcc/testsuite/gcc.target/i386/avx512f-mul-sf-zmm-4.c
> >>  create mode 100644 gcc/testsuite/gcc.target/i386/avx512f-mul-sf-zmm-5.c
> >>  create mode 100644 gcc/testsuite/gcc.target/i386/avx512f-mul-sf-zmm-6.c
> >>  create mode 100644 gcc/testsuite/gcc.target/i386/avx512vl-mul-sf-xmm-1.c
> >>  create mode 100644 gcc/testsuite/gcc.target/i386/avx512vl-mul-sf-ymm-1.c
> >>
> >> diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md
> >> index 411c78ae8d3..a73659e6bd2 100644
> >> --- a/gcc/config/i386/sse.md
> >> +++ b/gcc/config/i386/sse.md
> >> @@ -1754,6 +1754,30 @@
> >>     (set_attr "btver2_decode" "direct,double")
> >>     (set_attr "mode" "<MODE>")])
> >>
> >> +(define_insn "*mul<mode>3<mask_name>_bcst_1"
> >> +  [(set (match_operand:VF_AVX512 0 "register_operand" "=v")
> >> +       (mult:VF_AVX512
> >> +         (match_operand:VF_AVX512 1 "register_operand" "v")
> >> +         (vec_duplicate:VF_AVX512
> >> +            (match_operand:<ssescalarmode> 2 "memory_operand" "m"))))]
> >> +  "TARGET_AVX512F && <mask_mode512bit_condition>"
> >> +  "vmul<ssemodesuffix>\t{%2<avx512bcst>, %1,
> >> %0<mask_operand3>|%0<mask_operand3>, %1, %2<<avx512bcst>>}"
> >> +  [(set_attr "prefix" "evex")
> >> +   (set_attr "type" "ssemul")
> >> +   (set_attr "mode" "<MODE>")])
> >> +
> >> +(define_insn "*mul<mode>3<mask_name>_bcst_2"
> >> +  [(set (match_operand:VF_AVX512 0 "register_operand" "=v")
> >> +       (mult:VF_AVX512
> >> +         (vec_duplicate:VF_AVX512
> >> +            (match_operand:<ssescalarmode> 1 "memory_operand" "m"))
> >> +         (match_operand:VF_AVX512 2 "register_operand" "v")))]
> >> +  "TARGET_AVX512F && <mask_mode512bit_condition>"
> >> +  "vmul<ssemodesuffix>\t{%1<avx512bcst>, %2,
> >> %0<mask_operand3>|%0<mask_operand3>, %2, %1<<avx512bcst>>}"
> >> +  [(set_attr "prefix" "evex")
> >> +   (set_attr "type" "ssemul")
> >> +   (set_attr "mode" "<MODE>")])
> >
> > Do we really need two patterns here? IIRC, the compiler canonicalizes
> > commutative binops so that they have memory operand in the second
> > place. We have vec_duplicate here, so this may not be the case, but
> > please investigate if we really need two patterns for commutative
> > binops.
> >
>
> Only one pattern is needed.   For
>
>    (set (reg:V16SF 89) (vec_duplicate:V16SF (reg:SF 91)))
>    (set (reg:V16SF 95) (mult:V16SF (reg:V16SF 87) (reg:V16SF 89)))
>
> combiner prefers
>
>    (set (reg:V16SF 95)
>           (mult:V16SF
>             (vec_duplicate:V16SF (reg:SF 91))
>             (reg:V16SF 87)))
>
> instead of
>
>    (set (reg:V16SF 95)
>           (mult:V16SF
>             (reg:V16SF 87)
>             (vec_duplicate:V16SF (reg:SF 91))))
>
> commutation is performed at
>
> (set (reg:V16SF 95) (mult:V16SF (reg:V16SF 87) (reg:V16SF 89)))
>
> Here is the updated patch.  OK for trunk?

No need for a big comment, this is due to RTX operator precedence in
commutative operators..

OK with the above change.

Please also remove plus part from

*<plusminus_insn><mode>3<mask_name>_bcst_1

and rename it together with

*add<mode>3<mask_name>_bcst_2

to ..._bcst, without suffix.

Thanks,
Uros.

>
> --
> H.J.