[Bug rtl-optimization/105513] New: [9/10/11/12/13 Regression] Unnecessary SSE spill

[Bug rtl-optimization/105513] New: [9/10/11/12/13 Regression] Unnecessary SSE spill

[amonakov at gcc dot gnu.org]

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

            Bug ID: 105513
           Summary: [9/10/11/12/13 Regression] Unnecessary SSE spill
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Keywords: missed-optimization, ra
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: amonakov at gcc dot gnu.org
  Target Milestone: ---
            Target: x86_64-*-* i?86-*-*

Minimized from PR 105504.

Compile with -O2 -mtune=haswell -mavx (other -mtune variants are affected too):

static int as_int(float x)
{
    return (union{float x; int i;}){x}.i;
}

float f(double y, float x)
{
    int i = as_int(x);
    if (__builtin_expect(i > 99, 0)) return 0;
    if (i*2u < 77) if (i==2) return 0;
    return y*x;
}

GCC moves 'x' to 'i' via stack and then reloads from stack again when computing
'y*x':

f:
        vmovss  DWORD PTR [rsp-4], xmm1
        mov     eax, DWORD PTR [rsp-4]
        cmp     eax, 99
        jg      .L5
        lea     edx, [rax+rax]
        cmp     edx, 76
        ja      .L6
        cmp     eax, 2
        je      .L5
.L6:
        vcvtss2sd       xmm1, xmm1, DWORD PTR [rsp-4]
        vmulsd  xmm0, xmm1, xmm0
        vcvtsd2ss       xmm0, xmm0, xmm0
        ret
.L5:
        vxorps  xmm0, xmm0, xmm0
        ret

This is a regression relative to gcc-8. Interestingly, flipping '0' to '1' in
__builtin_expect (i.e. making early exit likely) results in good code, so
perhaps RA costing takes block probabilities backwards somewhere?

[Bug rtl-optimization/105513] [9/10/11/12/13 Regression] Unnecessary SSE spill since r9-5748-g1d4b4f4979171ef0

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
            Summary|[9/10/11/12/13 Regression]  |[9/10/11/12/13 Regression]
                   |Unnecessary SSE spill       |Unnecessary SSE spill since
                   |                            |r9-5748-g1d4b4f4979171ef0
                 CC|                            |marxin at gcc dot gnu.org,
                   |                            |ubizjak at gmail dot com
   Last reconfirmed|                            |2022-05-09
     Ever confirmed|0                           |1

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
Likely started with r9-5748-g1d4b4f4979171ef0.

[Bug target/105513] [9/10/11/12/13 Regression] Unnecessary SSE spill since r9-5748-g1d4b4f4979171ef0

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|rtl-optimization            |target
   Target Milestone|---                         |9.5

[Bug target/105513] [9/10/11/12/13 Regression] Unnecessary SSE spill since r9-5748-g1d4b4f4979171ef0

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
            Version|unknown                     |12.1.0

[Bug target/105513] [9/10/11/12/13 Regression] Unnecessary SSE spill since r9-5748-g1d4b4f4979171ef0

[crazylht at gmail dot com]

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

--- Comment #2 from Hongtao.liu <crazylht at gmail dot com> ---
Just note #c4 in pr105504 also solve this issue.

>Another possible solution is add a little bit dislike for "m" alternative(like ?m) to avoid potential spill.

[Bug target/105513] [9/10/11/12/13 Regression] Unnecessary SSE spill since r9-5748-g1d4b4f4979171ef0

[ubizjak at gmail dot com]

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

--- Comment #3 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Hongtao.liu from comment #2)
> Just note #c4 in pr105504 also solve this issue.
> 
> >Another possible solution is add a little bit dislike for "m" alternative(like ?m) to avoid potential spill.

There is nothing wrong with the *extendsfdf2 insn description, the costing
issue should be fixed elsewhere, probably in the generic code.

[Bug target/105513] [9/10/11/12/13 Regression] Unnecessary SSE spill since r9-5748-g1d4b4f4979171ef0

[crazylht at gmail dot com]

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

--- Comment #4 from Hongtao.liu <crazylht at gmail dot com> ---
For pattern supports 'm' alternative, mem_cost is frequency which is quite low
compared to pp->costs (ira_register_move_cost[mode][rclass][hard_reg_class]) *
frequency)

For x86 backend even gpr->gpr cost is 2 which should be 2 * frequency in
ira_cost,  I think we should somehow increase costs for REG_P (op) but choose
memory alternative.

 928                  if (op_class == NO_REGS)
 929                    /* Although we don't need insn to reload from
 930                       memory, still accessing memory is usually more
 931                       expensive than a register.  */
 932                    pp->mem_cost = frequency;

[Bug target/105513] [9/10/11/12/13 Regression] Unnecessary SSE spill since r9-5748-g1d4b4f4979171ef0

[crazylht at gmail dot com]

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

--- Comment #5 from Hongtao.liu <crazylht at gmail dot com> ---
And for constraint like 'vm', it's different from 'v,m' in calculating mem_cost
which will impact RA when op is REG_P. For 'v,m' mem_cost is just 1 *
frequency, but for 'vm' mem_cost is much bigger(memory_move_cost - 1) *
frequency. It means the impact of writing 'v,m' instead of 'vm' makes RA more
likely to choose mem instead of register.

 928                  if (op_class == NO_REGS)
 929                    /* Although we don't need insn to reload from
 930                       memory, still accessing memory is usually more
 931                       expensive than a register.  */
 932                    pp->mem_cost = frequency;
 933                  else
 934                    /* If the alternative actually allows memory, make
 935                       things a bit cheaper since we won't need an
 936                       extra insn to load it.  */
 937                    pp->mem_cost
 938                      = ((out_p ? ira_memory_move_cost[mode][op_class][0] :
0)
 939                         + (in_p ? ira_memory_move_cost[mode][op_class][1]
: 0)
 940=>                       - allows_mem[i]) * frequency;

[Bug target/105513] [9/10/11/12/13 Regression] Unnecessary SSE spill since r9-5748-g1d4b4f4979171ef0

[crazylht at gmail dot com]

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

--- Comment #6 from Hongtao.liu <crazylht at gmail dot com> ---
Also notice a intersting case impacted by a separate m alternatvie.

typedef long v2di __attribute__((vector_size(16)));

v2di
foo (v2di a)
{
  a[1] = 1113;
  return a;
}

with -O2 gcc generates

foo(long __vector(2)):
        movhps  .LC0(%rip), %xmm0
        ret
.LC0:
        .quad   1113

llvm has

foo(long __vector(2)):                            # @foo(long __vector(2))
        movl    $1113, %eax                     # imm = 0x459
        movq    %rax, %xmm1
        punpcklqdq      %xmm1, %xmm0            # xmm0 = xmm0[0],xmm1[0]
        retq

Microbenchmark show both both sequences are almost as fast, really don't know
which is better.

[Bug target/105513] [9/10/11/12/13 Regression] Unnecessary SSE spill since r9-5748-g1d4b4f4979171ef0

[amonakov at gcc dot gnu.org]

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

--- Comment #7 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
The second sequence is 3 uops vs 1/2 (issued/executed) uops in first, and on
Haswell and Skylake it ties up port 5 for two cycles.

Unclear if you're microbenchmarking latency or throughput, but in any case on
Haswell and Skylake you should see a close to 2x difference.

[Bug target/105513] [9/10/11/12/13 Regression] Unnecessary SSE spill since r9-5748-g1d4b4f4979171ef0

[crazylht at gmail dot com]

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

--- Comment #8 from Hongtao.liu <crazylht at gmail dot com> ---
(In reply to Alexander Monakov from comment #7)
> The second sequence is 3 uops vs 1/2 (issued/executed) uops in first, and on
> Haswell and Skylake it ties up port 5 for two cycles.
> 
> Unclear if you're microbenchmarking latency or throughput, but in any case
> on Haswell and Skylake you should see a close to 2x difference.

I'm counting clocksticks, and thought a load may take more latency.

#include <stdio.h>
#include <stdlib.h>
#include <x86intrin.h>

#define LOOP 1000000000
typedef long v2di __attribute__((vector_size(16)));
typedef int v4si __attribute__((vector_size(16)));

v2di
__attribute__ ((noipa))
foo (v2di a)
{
        a[1] = 111113;
        return a;
}

void
__attribute__ ((noipa))
foo1 (v2di a)
{
}

int
main ()
{
  int i;
  unsigned long long start, end;
  unsigned long long diff;
  unsigned int aux;

  start = __rdtscp (&aux);
  v2di b = __extension__ (v2di){111, 222};
  for (i = 0; i < LOOP; i++)
    {
      v2di a = foo (b);
      foo1 (a);
    }
  end = __rdtscp (&aux);
  diff = end - start;
  printf ("alterna: %lld\n", diff);

  return 0;
}

[Bug target/105513] [10/11/12/13 Regression] Unnecessary SSE spill since r9-5748-g1d4b4f4979171ef0

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|9.5                         |10.4

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 9 branch is being closed

[Bug target/105513] [10/11/12/13 Regression] Unnecessary SSE spill since r9-5748-g1d4b4f4979171ef0

[cvs-commit at gcc dot gnu.org]

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

--- 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:5e005393d4ff0a428c5f55b9ba7f65d6078a7cf5

commit r13-1009-g5e005393d4ff0a428c5f55b9ba7f65d6078a7cf5
Author: liuhongt <hongtao.liu@intel.com>
Date:   Mon May 30 15:30:51 2022 +0800

    Disparages SSE_REGS alternatives sligntly with ?v instead of *v in
*mov{si,di}_internal.

    So alternative v won't be igored in record_reg_classess.

    Similar for *r alternatives in some vector patterns.

    It helps testcase in the PR, also RA now makes better decisions for
    gcc.target/i386/extract-insert-combining.c

            movd    %esi, %xmm0
            movd    %edi, %xmm1
    -       movl    %esi, -12(%rsp)
            paddd   %xmm0, %xmm1
            pinsrd  $0, %esi, %xmm0
            paddd   %xmm1, %xmm0

    The patch has no big impact on SPEC2017 for both O2 and Ofast
    march=native run.

    And I noticed there's some changes in SPEC2017 from code like

    mov mem, %eax
    vmovd %eax, %xmm0
    ..
    mov %eax, 64(%rsp)

    to

    vmovd mem, %xmm0
    ..
    vmovd %xmm0, 64(%rsp)

    Which should be exactly what we want?

    gcc/ChangeLog:

            PR target/105513
            PR target/105504
            * config/i386/i386.md (*movsi_internal): Change alternative
            from *v to ?v.
            (*movdi_internal): Ditto.
            * config/i386/sse.md (vec_set<mode>_0): Change alternative *r
            to ?r.
            (*vec_extractv4sf_mem): Ditto.
            (*vec_extracthf): Ditto.

    gcc/testsuite/ChangeLog:

            * gcc.target/i386/pr105513-1.c: New test.
            * gcc.target/i386/extract-insert-combining.c: Add new
            scan-assembler-not for spill.

[Bug target/105513] [10/11/12/13 Regression] Unnecessary SSE spill since r9-5748-g1d4b4f4979171ef0

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.4                        |10.5

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 10.4 is being released, retargeting bugs to GCC 10.5.

[Bug target/105513] [11/12/13/14 Regression] Unnecessary SSE spill since r9-5748-g1d4b4f4979171ef0

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.5                        |11.5

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10 branch is being closed.

[Bug target/105513] [11/12/13/14 Regression] Unnecessary SSE spill since r9-5748-g1d4b4f4979171ef0

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|11.5                        |13.0

--- Comment #13 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Fixed for GCC 13.