[Bug rtl-optimization/47581] New: [4.6 regression] Unnecessary adjustments to stack pointer

[Bug rtl-optimization/47581] New: [4.6 regression] Unnecessary adjustments to stack pointer

[tony.poppleton at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

           Summary: [4.6 regression] Unnecessary adjustments to stack
                    pointer
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: tony.poppleton@gmail.com


Whilst investigating PR4079 (which affects PPC), I found some strange
adjustments to the stack pointer when compiling with 4.6.0 (20110129) on x86.

For reference, the C code from that PR is:

unsigned mulh(unsigned a, unsigned b)
{
        return ((unsigned long long)a * (unsigned long long)b) >> 32;
}

On 4.5.2 using "-O2 -m32 -fomit-frame-pointer", this produced the following
succinct code:
mulh:
        movl    8(%esp), %eax
        mull    4(%esp)
        movl    %edx, %eax
        ret
        .size   mulh, .-mulh
        .ident  "GCC: (GNU) 4.5.2"

However on 4.6.0 with the same arguments:
mulh:
.LFB0:
        .cfi_startproc
        subl    $4, %esp           <== isn't this unnecessary?
        .cfi_def_cfa_offset 8
        movl    12(%esp), %eax     <== this could just be 8(%esp)
        mull    8(%esp)            <== this could just be 4(%esp)
        addl    $4, %esp           <== isn't this unnecessary?
        .cfi_def_cfa_offset 4
        movl    %edx, %eax
        ret
        .cfi_endproc
.LFE0:
        .size   mulh, .-mulh
        .ident  "GCC: (GNU) 4.6.0 20110129 (experimental)"

[Bug rtl-optimization/47581] [4.6 regression] Unnecessary adjustments to stack pointer

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.6.0

--- Comment #1 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-02-01 22:55:51 UTC ---
There is a duplicate of this bug somewhere, the issue is the stack re-alignment
code.

[Bug middle-end/47581] [4.6 regression] Unnecessary adjustments to stack pointer

[hjl.tools at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011.02.01 23:20:29
                 CC|                            |rth at gcc dot gnu.org
     Ever Confirmed|0                           |1

--- Comment #2 from H.J. Lu <hjl.tools at gmail dot com> 2011-02-01 23:20:29 UTC ---
It is caused by revision 165240:

http://gcc.gnu.org/ml/gcc-cvs/2010-10/msg00423.html

[Bug middle-end/47581] [4.6 regression] Unnecessary adjustments to stack pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

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

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-02-02 11:23:18 UTC ---
Before Richard's commit apparently expand_one_var wasn't updating
stack_alignment_needed, just stack_alignment_estimated, now it updates both.
As PREFERRED_STACK_BOUNDARY is 128 (the default), ix86_minimum_alignment:
  if (TARGET_64BIT || align != 64 || ix86_preferred_stack_boundary >= 64)
    return align;
returns 64 instead of 32.

So, to fix this, either the stack_alignment_needed updating in expand_one_var
isn't strictly necessary and we could undo that part of the changes.

Or return 32 as MINIMUM_ALIGNMENT for DImode long long even for
ix86_preferred_stack_boundary >= 64.

Or we could perhaps the
  /* Align start of frame for local function.  */
  offset = (offset + stack_alignment_needed - 1) & -stack_alignment_needed;
in ix86_compute_frame_layout perform only if
  if (offset != frame->sse_reg_save_offset
      || size != 0
      || !current_function_is_leaf
      || cfun->calls_alloca
      || ix86_current_function_calls_tls_descriptor)
(i.e. if frame size is 0 and no outgoing calls, it doesn't make sense to
align).

[Bug middle-end/47581] [4.6 regression] Unnecessary adjustments to stack pointer

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

--- Comment #4 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-02-02 12:12:23 UTC ---
(In reply to comment #3)
> Before Richard's commit apparently expand_one_var wasn't updating
> stack_alignment_needed, just stack_alignment_estimated, now it updates both.
> As PREFERRED_STACK_BOUNDARY is 128 (the default), ix86_minimum_alignment:
>   if (TARGET_64BIT || align != 64 || ix86_preferred_stack_boundary >= 64)
>     return align;
> returns 64 instead of 32.
> 
> So, to fix this, either the stack_alignment_needed updating in expand_one_var
> isn't strictly necessary and we could undo that part of the changes.
> 
> Or return 32 as MINIMUM_ALIGNMENT for DImode long long even for
> ix86_preferred_stack_boundary >= 64.
> 
> Or we could perhaps the
>   /* Align start of frame for local function.  */
>   offset = (offset + stack_alignment_needed - 1) & -stack_alignment_needed;
> in ix86_compute_frame_layout perform only if
>   if (offset != frame->sse_reg_save_offset
>       || size != 0
>       || !current_function_is_leaf
>       || cfun->calls_alloca
>       || ix86_current_function_calls_tls_descriptor)
> (i.e. if frame size is 0 and no outgoing calls, it doesn't make sense to
> align).

I think we align the stack for reload (which might still happen without
outgoing calls, simply with enough register pressure).

[Bug middle-end/47581] [4.6 regression] Unnecessary adjustments to stack pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-02-02 12:25:01 UTC ---
But then wouldn't be size != 0?

At least when I try -O2 -m32:

int
foo (int x, int y)
{
  long long a = (x + 0LL) * y;
  long long b = (x - 63LL) * ((y | 12) + 67LL);
  long long c = (x - 27LL) * ((y | 17) + 12);
  long long d = (x - 79LL) * ((y | 12) + 18);
  long long e = (x - 28LL) * ((y | 21) + 34LL);
  long long f = (x - 77LL) * ((y | 18) + 39LL);
  return (a >> 10) + (b >> 8) + (c >> 6) + (d >> 24) + (e >> 17) + (f >> 39);
}

size is 0 until IRA time (no stack slots needed) and thus with the patch it
keeps not aligning the offset and then when some pseudos are spilled the
function is called again, at that point already get_frame_size is non-zero and
it allocates bigger stack.  reload iterates as long as
verify_initial_elimination_offsets fails or as long as frame size changes, and
  if (insns_need_reload != 0 || something_needs_elimination
      || something_needs_operands_changed)
    {
      HOST_WIDE_INT old_frame_size = get_frame_size ();

      reload_as_needed (global);

      gcc_assert (old_frame_size == get_frame_size ());

      gcc_assert (verify_initial_elim_offsets ());
    }
should IMHO make sure the patch can work properly (because it only changes
frame offsets if get_frame_size () changed and these asserts assure they don't.

[Bug middle-end/47581] [4.6 regression] Unnecessary adjustments to stack pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-02-02 13:52:42 UTC ---
Created attachment 23213
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=23213
gcc46-pr47581.patch

This patch indeed fixes the testcase and passed bootstrap/regtest on
x86_64-linux and i686-linux.

[Bug middle-end/47581] [4.6 regression] Unnecessary adjustments to stack pointer

[hjl.tools at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

--- Comment #7 from H.J. Lu <hjl.tools at gmail dot com> 2011-02-02 14:18:22 UTC ---
(In reply to comment #6)
> Created attachment 23213 [details]
> gcc46-pr47581.patch
> 
> This patch indeed fixes the testcase and passed bootstrap/regtest on
> x86_64-linux and i686-linux.

Update both stack_alignment_needed and stack_alignment_estimated
in expand_one_var may lead missed optimization like this. We
should delay stack_alignment_needed as much as we can. Setting
stack_alignment_estimated should be good enough before
stack_realign_finalized is set.

We should find out why expand_one_var needs to set
stack_alignment_needed.  I think this is a real bug.

[Bug middle-end/47581] [4.6 regression] Unnecessary adjustments to stack pointer

[hjl.tools at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

--- Comment #8 from H.J. Lu <hjl.tools at gmail dot com> 2011-02-02 14:57:22 UTC ---
For this tescase, gcc 4.4 generates the most efficient code
while 4.5/4.6 become worse and worse:

[hjl@gnu-6 gcc]$ cat x.i 
unsigned mulh(unsigned a, unsigned b)
{
  unsigned long long l __attribute__ ((aligned(32)))
    =((unsigned long long)a * (unsigned long long)b) >> 32;
  return l;
}
[hjl@gnu-6 gcc]$ /usr/gcc-4.4/bin/gcc -O2 -S -fomit-frame-pointer x.i -m32
[hjl@gnu-6 gcc]$ cat x.s 
    .file    "x.i"
    .text
    .p2align 4,,15
.globl mulh
    .type    mulh, @function
mulh:
    movl    8(%esp), %eax
    mull    4(%esp)
    movl    %edx, %eax
    ret
    .size    mulh, .-mulh
    .ident    "GCC: (GNU) 4.4.4"
    .section    .note.GNU-stack,"",@progbits
[hjl@gnu-6 gcc]$ /usr/gcc-4.5/bin/gcc -O2 -S -fomit-frame-pointer x.i -m32
[hjl@gnu-6 gcc]$ cat x.s
    .file    "x.i"
    .text
    .p2align 4,,15
.globl mulh
    .type    mulh, @function
mulh:
    pushl    %ebp
    movl    %esp, %ebp
    movl    12(%ebp), %eax
    mull    8(%ebp)
    popl    %ebp
    movl    %edx, %eax
    ret
    .size    mulh, .-mulh
    .ident    "GCC: (GNU) 4.5.1 20100507 (prerelease) [gcc-4_5-branch revision
159167]"
    .section    .note.GNU-stack,"",@progbits
[hjl@gnu-6 gcc]$ ./xgcc -B./  -O2 -S -fomit-frame-pointer x.i -m32
[hjl@gnu-6 gcc]$ cat x.s
    .file    "x.i"
    .text
    .p2align 4,,15
    .globl    mulh
    .type    mulh, @function
mulh:
.LFB0:
    .cfi_startproc
    pushl    %ebp
    .cfi_def_cfa_offset 8
    .cfi_offset 5, -8
    movl    %esp, %ebp
    .cfi_def_cfa_register 5
    andl    $-32, %esp
    movl    12(%ebp), %eax
    mull    8(%ebp)
    leave
    .cfi_restore 5
    .cfi_def_cfa 4, 4
    movl    %edx, %eax
    ret
    .cfi_endproc
.LFE0:
    .size    mulh, .-mulh
    .ident    "GCC: (GNU) 4.6.0 20110131 (experimental)"
    .section    .note.GNU-stack,"",@progbits
[hjl@gnu-6 gcc]$

[Bug middle-end/47581] [4.5/4.6 regression] Unnecessary adjustments to stack pointer

[hjl.tools at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

H.J. Lu <hjl.tools at gmail dot com> changed:

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

--- Comment #9 from H.J. Lu <hjl.tools at gmail dot com> 2011-02-02 15:05:10 UTC ---
(In reply to comment #8)
> For this tescase, gcc 4.4 generates the most efficient code
> while 4.5/4.6 become worse and worse:
> 
> [hjl@gnu-6 gcc]$ cat x.i 
> unsigned mulh(unsigned a, unsigned b)
> {
>   unsigned long long l __attribute__ ((aligned(32)))
>     =((unsigned long long)a * (unsigned long long)b) >> 32;
>   return l;
> }

This is caused by revision 146817:

http://gcc.gnu.org/ml/gcc-cvs/2009-04/msg01459.html

[Bug middle-end/47581] [4.5/4.6 regression] Unnecessary adjustments to stack pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-02-02 15:14:45 UTC ---
As #c8 is caused by a different patch and is much lower priority (because only
very few people use explicitly aligned long longs on i?86), please track it
separately instead of overloading this bug.

[Bug middle-end/47581] [4.5/4.6 regression] Unnecessary adjustments to stack pointer

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug middle-end/47581] [4.5/4.6 regression] Unnecessary adjustments to stack pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-02-15 11:46:14 UTC ---
Author: jakub
Date: Tue Feb 15 11:46:10 2011
New Revision: 170170

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=170170
Log:
    PR middle-end/47581
    * config/i386/i386.c (ix86_compute_frame_size): Don't align offset
    if frame size is 0 in a leaf function.

    * gcc.target/i386/pr47581.c: New test.

Added:
    trunk/gcc/testsuite/gcc.target/i386/pr47581.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.c
    trunk/gcc/testsuite/ChangeLog

[Bug middle-end/47581] [4.5 regression] Unnecessary adjustments to stack pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
      Known to work|                            |4.6.0
            Summary|[4.5/4.6 regression]        |[4.5 regression]
                   |Unnecessary adjustments to  |Unnecessary adjustments to
                   |stack pointer               |stack pointer
      Known to fail|4.6.0                       |

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-02-15 12:11:30 UTC ---
Fixed on the trunk so far.

[Bug middle-end/47581] [4.5 regression] Unnecessary adjustments to stack pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.6.0                       |4.6.1

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-03-25 19:52:50 UTC ---
GCC 4.6.0 is being released, adjusting target milestone.

[Bug middle-end/47581] [4.5 regression] Unnecessary adjustments to stack pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.6.1                       |4.6.2

--- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-06-27 12:33:04 UTC ---
GCC 4.6.1 is being released.

[Bug middle-end/47581] [4.5 regression] Unnecessary adjustments to stack pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.6.2                       |4.6.3

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-10-26 17:13:48 UTC ---
GCC 4.6.2 is being released.

[Bug middle-end/47581] [4.5 regression] Unnecessary adjustments to stack pointer

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.6.3                       |4.6.4

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-03-01 14:38:55 UTC ---
GCC 4.6.3 is being released.

[Bug middle-end/47581] [4.5 regression] Unnecessary adjustments to stack pointer

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.6.4                       |4.5.4

[Bug middle-end/47581] [4.5 regression] Unnecessary adjustments to stack pointer

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47581

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|4.5.4                       |4.6.0

--- Comment #17 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-07-02 10:33:55 UTC ---
Fixed in 4.6.0, the 4.5 branch is being closed.