[Bug target/97147] New: GCC uses vhaddpd which is bad for latency

[Bug target/97147] New: GCC uses vhaddpd which is bad for latency

[rguenth at gcc dot gnu.org]

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

            Bug ID: 97147
           Summary: GCC uses vhaddpd which is bad for latency
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rguenth at gcc dot gnu.org
  Target Milestone: ---

typedef double v2df __attribute__((vector_size(16)));
double foo (v2df x, double y)
{
  return x[0] + x[1] + y;
}
double bar (v2df x, double y)
{
  return y + x[0] + x[1];
}

with -O2 -mavx2 -mtune=znver2 ends up generating

foo:
.LFB0:
        .cfi_startproc
        vhaddpd %xmm0, %xmm0, %xmm0
        vaddsd  %xmm1, %xmm0, %xmm0
        ret

bar:
.LFB1:
        .cfi_startproc
        vmovapd %xmm0, %xmm2
        vaddsd  %xmm1, %xmm0, %xmm0
        vunpckhpd       %xmm2, %xmm2, %xmm2
        vaddsd  %xmm2, %xmm0, %xmm0
        ret

where bar should be a _lot_ better according to Agner which says
that vhaddpd has a 4 uops, a latency of 7 cycles and a throughput of only
one per two cycles while both vunpckhpd and vaddsd fare a lot better here.
Coffee-lake isn't much better here.

Maybe we want to disable the V2DF instructions for most tunings somehow?

[Bug target/97147] GCC uses vhaddpd which is bad for latency

[crazylht at gmail dot com]

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

--- Comment #1 from Hongtao.liu <crazylht at gmail dot com> ---
(In reply to Richard Biener from comment #0)
> typedef double v2df __attribute__((vector_size(16)));
> double foo (v2df x, double y)
> {
>   return x[0] + x[1] + y;
> }
> double bar (v2df x, double y)
> {
>   return y + x[0] + x[1];
> }
> 
> with -O2 -mavx2 -mtune=znver2 ends up generating
> 
> foo:
> .LFB0:
>         .cfi_startproc
>         vhaddpd %xmm0, %xmm0, %xmm0
>         vaddsd  %xmm1, %xmm0, %xmm0
>         ret
> 
> bar:
> .LFB1:
>         .cfi_startproc
>         vmovapd %xmm0, %xmm2
>         vaddsd  %xmm1, %xmm0, %xmm0
>         vunpckhpd       %xmm2, %xmm2, %xmm2
>         vaddsd  %xmm2, %xmm0, %xmm0
>         ret
> 
> where bar should be a _lot_ better according to Agner which says
> that vhaddpd has a 4 uops, a latency of 7 cycles and a throughput of only
> one per two cycles while both vunpckhpd and vaddsd fare a lot better here.
> Coffee-lake isn't much better here.
> 
> Maybe we want to disable the V2DF instructions for most tunings somehow?

Bar is also better on CLX and ICL.

[Bug target/97147] GCC uses vhaddpd which is bad for latency

[crazylht at gmail dot com]

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

--- Comment #2 from Hongtao.liu <crazylht at gmail dot com> ---
Disable (define_insn "*sse3_haddv2df3_low" and (define_insn
"*sse3_hsubv2df3_low" seems to be ok.
But for foo1.

v2df foo1 (v2df x, v2df y)
{
  v2df a;
  a[0] = x[0] + x[1];
  a[1] = y[0] + y[1];
  return a;
}

it's 

  vhaddpd %xmm1, %xmm0, %xmm0
  ret

vs 

        movapd  xmm2, xmm0
        unpckhpd        xmm2, xmm2
        addsd   xmm0, xmm2
        movapd  xmm2, xmm1
        unpckhpd        xmm1, xmm1
        addsd   xmm1, xmm2
        unpcklpd        xmm0, xmm1
        ret

and note w/o vhaddpd, codegen can be optimized to 

        movapd  xmm2, xmm0
        unpcklpd        xmm2, xmm1
        unpckhpd        xmm0, xmm1
        addpd   xmm0, xmm2
        ret

Guess maybe it's better done in gimple level?

[Bug target/97147] GCC uses vhaddpd which is bad for latency

[rguenth at gcc dot gnu.org]

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Hongtao.liu from comment #2)
> Disable (define_insn "*sse3_haddv2df3_low" and (define_insn
> "*sse3_hsubv2df3_low" seems to be ok.
> But for foo1.
> 
> v2df foo1 (v2df x, v2df y)
> {
>   v2df a;
>   a[0] = x[0] + x[1];
>   a[1] = y[0] + y[1];
>   return a;
> }
> 
> it's 
> 
>   vhaddpd %xmm1, %xmm0, %xmm0
>   ret
> 
> vs 
> 
>         movapd  xmm2, xmm0
>         unpckhpd        xmm2, xmm2
>         addsd   xmm0, xmm2
>         movapd  xmm2, xmm1
>         unpckhpd        xmm1, xmm1
>         addsd   xmm1, xmm2
>         unpcklpd        xmm0, xmm1
>         ret
> 
> and note w/o vhaddpd, codegen can be optimized to 
> 
>         movapd  xmm2, xmm0
>         unpcklpd        xmm2, xmm1
>         unpckhpd        xmm0, xmm1
>         addpd   xmm0, xmm2
>         ret
> 
> Guess maybe it's better done in gimple level?

On GIMPLE we see the testcase basically unchanged from what the source does:

  _1 = BIT_FIELD_REF <x_7(D), 64, 0>;
  _2 = BIT_FIELD_REF <x_7(D), 64, 64>;
  _3 = _1 + _2;
  a_9 = BIT_INSERT_EXPR <a_8(D), _3, 0>;
  _4 = BIT_FIELD_REF <y_10(D), 64, 0>;
  _5 = BIT_FIELD_REF <y_10(D), 64, 64>;
  _6 = _4 + _5;
  a_11 = BIT_INSERT_EXPR <a_9, _6, 64>;
  return a_11;

vectorization fails in SLP discovery because we essentially see two lanes
operating on different vectors and we don't implement a way to shuffle
them together.

I think the full hadd define_insns are OK to keep, they really have special
arrangements (esp. the SFmode variants).  But the reductions to scalar
(*_low) seem unnecessary and penaltizing (maybe we can guard use of those
with a -mtune-ctl?).

I also see we're missing patterns for h{add,sub}ps (not sure if we can manage
to get combine to synthesize it).

[Bug target/97147] GCC uses vhaddpd which is bad for latency

[crazylht at gmail dot com]

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

--- Comment #4 from Hongtao.liu <crazylht at gmail dot com> ---
(In reply to Richard Biener from comment #3)
> (In reply to Hongtao.liu from comment #2)
> > Disable (define_insn "*sse3_haddv2df3_low" and (define_insn
> > "*sse3_hsubv2df3_low" seems to be ok.
> > But for foo1.
> > 
> > v2df foo1 (v2df x, v2df y)
> > {
> >   v2df a;
> >   a[0] = x[0] + x[1];
> >   a[1] = y[0] + y[1];
> >   return a;
> > }
> > 
> > it's 
> > 
> >   vhaddpd %xmm1, %xmm0, %xmm0
> >   ret
> > 
> > vs 
> > 
> >         movapd  xmm2, xmm0
> >         unpckhpd        xmm2, xmm2
> >         addsd   xmm0, xmm2
> >         movapd  xmm2, xmm1
> >         unpckhpd        xmm1, xmm1
> >         addsd   xmm1, xmm2
> >         unpcklpd        xmm0, xmm1
> >         ret
> > 
> > and note w/o vhaddpd, codegen can be optimized to 
> > 
> >         movapd  xmm2, xmm0
> >         unpcklpd        xmm2, xmm1
> >         unpckhpd        xmm0, xmm1
> >         addpd   xmm0, xmm2
> >         ret
> > 
> > Guess maybe it's better done in gimple level?
> 
> On GIMPLE we see the testcase basically unchanged from what the source does:
> 
>   _1 = BIT_FIELD_REF <x_7(D), 64, 0>;
>   _2 = BIT_FIELD_REF <x_7(D), 64, 64>;
>   _3 = _1 + _2;
>   a_9 = BIT_INSERT_EXPR <a_8(D), _3, 0>;
>   _4 = BIT_FIELD_REF <y_10(D), 64, 0>;
>   _5 = BIT_FIELD_REF <y_10(D), 64, 64>;
>   _6 = _4 + _5;
>   a_11 = BIT_INSERT_EXPR <a_9, _6, 64>;
>   return a_11;
> 
> vectorization fails in SLP discovery because we essentially see two lanes
> operating on different vectors and we don't implement a way to shuffle
> them together.
> 
> I think the full hadd define_insns are OK to keep, they really have special
> arrangements (esp. the SFmode variants).  But the reductions to scalar
> (*_low) seem unnecessary and penaltizing (maybe we can guard use of those
> with a -mtune-ctl?).
> 

Yes, i'm add a tune to enabled v2df vector reduction and defaut disabled for
all processors.

> I also see we're missing patterns for h{add,sub}ps (not sure if we can manage
> to get combine to synthesize it).

You mean (define_insn "sse3_h<insn>v4sf3"?

[Bug target/97147] GCC uses vhaddpd which is bad for latency

[rguenther at suse dot de]

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

--- Comment #5 from rguenther at suse dot de <rguenther at suse dot de> ---
On Tue, 17 Aug 2021, crazylht at gmail dot com wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97147
> 
> --- Comment #4 from Hongtao.liu <crazylht at gmail dot com> ---
> (In reply to Richard Biener from comment #3)
> > I also see we're missing patterns for h{add,sub}ps (not sure if we can manage
> > to get combine to synthesize it).
> 
> You mean (define_insn "sse3_h<insn>v4sf3"?

Yeah...  stupid macros ;)

[Bug target/97147] GCC uses vhaddpd which is bad for latency

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 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:97d51c1764554fcef05fe94ee6445f5d2252bcff

commit r12-2981-g97d51c1764554fcef05fe94ee6445f5d2252bcff
Author: liuhongt <hongtao.liu@intel.com>
Date:   Tue Aug 17 13:11:26 2021 +0800

    Add x86 tune to enable v2df vector reduction by paddpd.

    The tune is disabled by default.

    gcc/ChangeLog:

            PR target/97147
            * config/i386/i386.h (TARGET_V2DF_REDUCTION_PREFER_HADDPD):
            New macro.
            * config/i386/sse.md (*sse3_haddv2df3_low): Add
            TARGET_V2DF_REDUCTION_PREFER_HADDPD.
            (*sse3_hsubv2df3_low): Ditto.
            * config/i386/x86-tune.def
            (X86_TUNE_V2DF_REDUCTION_PREFER_HADDPD): New tune.

    gcc/testsuite/ChangeLog:

            PR target/97147
            * gcc.target/i386/pr54400.c: Adjust testcase.
            * gcc.target/i386/pr94147.c: New test.

[Bug target/97147] GCC uses vhaddpd which is bad for latency

[crazylht at gmail dot com]

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

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

[Bug target/97147] GCC uses vhaddpd which is bad for latency

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED
      Known to work|                            |12.0

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.