[Bug target/37010] New: -Os passes __m128 on stack with wrong alignment

[Bug target/37010] New: -Os passes __m128 on stack with wrong alignment

[hjl dot tools at gmail dot com]

-Os passes __m128 on stack with wrong alignment:

bash-3.2$ cat x.c
#include <emmintrin.h>

extern void foo (__m128 x, __m128 y ,__m128 z ,__m128 a, int size);

void
bar (void)
{
  __m128 x = { 1.0 };
  foo (x, x, x, x, 5);
}
bash-3.2$ /export/build/gnu/gcc-work/build-x86_64-linux/gcc/xgcc
-B/export/build/gnu/gcc-work/build-x86_64-linux/gcc/    -Os  -msse2  -m32 x.c
-S 
bash-3.2$ cat x.s
        .file   "x.c"
        .text
.globl bar
        .type   bar, @function
bar:
        pushl   %ebp
        movl    %esp, %ebp
        subl    $20, %esp
        movss   .LC0, %xmm0
        pushl   $5
        subl    $16, %esp
        movups  %xmm0, (%esp)
^^^^^^^^^^^^^^^^ This should be aligned at 16byte with movaps.
        movaps  %xmm0, %xmm2
        movaps  %xmm0, %xmm1
        call    foo
        addl    $32, %esp
        leave
        ret


-- 
           Summary: -Os passes __m128 on stack with wrong alignment
           Product: gcc
           Version: 4.4.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: hjl dot tools at gmail dot com
GCC target triplet: i686-pc-linux-gnu


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

[Bug target/37010] -Os passes __m128 on stack with wrong alignment

[hjl dot tools at gmail dot com]

------- Comment #1 from hjl dot tools at gmail dot com  2008-08-03 00:37 -------
A patch is posted at

http://gcc.gnu.org/ml/gcc-patches/2008-08/msg00123.html


-- 

hjl dot tools at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |Joey dot ye at intel dot
                   |                            |com, xuepeng dot guo at
                   |                            |intel dot com
                URL|                            |http://gcc.gnu.org/ml/gcc-
                   |                            |patches/2008-
                   |                            |08/msg00123.html


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

[Bug target/37010] -Os passes __m128 on stack with wrong alignment

[hjl dot tools at gmail dot com]

------- Comment #2 from hjl dot tools at gmail dot com  2008-08-03 18:18 -------
(In reply to comment #1)
> A patch is posted at
> 
> http://gcc.gnu.org/ml/gcc-patches/2008-08/msg00123.html
> 

This patch is incorrect. The problem is between ACCUMULATE_OUTGOING_ARGS,
ix86_compute_frame_layout and ix86_expand_prologue. Joey, what do
you think?


-- 

hjl dot tools at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|middle-end                  |target


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

[Bug target/37010] -Os passes __m128 on stack with wrong alignment

[hjl dot tools at gmail dot com]

------- Comment #3 from hjl dot tools at gmail dot com  2008-08-03 18:40 -------
Joey, when we compute frame layout, we don't count the duplicated
return address pushed onto stack when DRAP is used. Also when we
push return address, shouldn't we use -UNITS_PER_WORD, instead of
-(STACK_BOUNDARY / BITS_PER_UNIT))? On MacOS, STACK_BOUNDARY is
128 on ia32. Does this patch make sense?

--- ./i386.c.drap       2008-08-03 09:50:05.000000000 -0700
+++ ./i386.c    2008-08-03 11:36:40.000000000 -0700
@@ -7291,6 +7291,10 @@ ix86_compute_frame_layout (struct ix86_f
   if (stack_realign_fp)
     offset = (offset + stack_alignment_needed -1) & -stack_alignment_needed;

+  /* Duplicated return address when DRAP is used.  */
+  if (crtl->drap_reg && crtl->stack_realign_needed)
+    offset += UNITS_PER_WORD;
+
   /* Register save area */
   offset += frame->nregs * UNITS_PER_WORD;

@@ -7692,8 +7696,7 @@ ix86_expand_prologue (void)
         expand_builtin_return_addr etc.  */
       x = crtl->drap_reg;
       x = gen_frame_mem (Pmode,
-                         plus_constant (x,
-                                       -(STACK_BOUNDARY / BITS_PER_UNIT)));
+                         plus_constant (x, -UNITS_PER_WORD));
       insn = emit_insn (gen_push (x));
       RTX_FRAME_RELATED_P (insn) = 1;
     }


-- 

hjl dot tools at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at ucw dot cz


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

[Bug target/37010] -Os passes __m128 on stack with wrong alignment

[hjl dot tools at gmail dot com]

------- Comment #4 from hjl dot tools at gmail dot com  2008-08-03 20:11 -------
A run-time testcase:

bash-3.2$ cat y.c
/* PR middle-end/37010 */
/* { dg-do run { target { { i?86-*-* x86_64-*-* } && ilp32 } } } */
/* { dg-options "-msse2" } */

typedef __PTRDIFF_TYPE__ ptrdiff_t;
extern void abort (void);

int
__attribute__ ((noinline))
check_int (int *i, int align)
{
  *i = 20;
  if ((((ptrdiff_t) i) & (align - 1)) != 0)
    {
      abort ();
    }
  return *i;
}
typedef float __m128 __attribute__ ((__vector_size__ (16), __may_alias__));

typedef int aligned __attribute__((aligned(16)));

__m128 r;

void
__attribute__ ((noinline))
foo (__m128 x, __m128 y ,__m128 z ,__m128 a, int size)
{
  aligned i;

  if (check_int (&i, __alignof__(i)) != i)
    abort ();
  r = a;
}

int
main (void)
{
  __m128 x = { 1.0 };

  foo (x, x, x, x, 5);

  if (__builtin_memcmp (&r, &x, sizeof (r)))
    abort ();

  return 0;
}
bash-3.2$ make y
/export/build/gnu/gcc-avx/build-x86_64-linux/gcc/xgcc
-B/export/build/gnu/gcc-avx/build-x86_64-linux/gcc/ -m32 -msse2 -DDEBUG -Os
-mpreferred-stack-boundary=2    y.c   -o y
bash-3.2$ ./y
Segmentation fault
bash-3.2$ /export/build/gnu/gcc-avx/build-x86_64-linux/gcc/xgcc
-B/export/build/gnu/gcc-avx/build-x86_64-linux/gcc/ -m32 -msse2 -DDEBUG -O
-mpreferred-stack-boundary=2    y.c   -o y
bash-3.2$ ./y
bash-3.2$ 


-- 


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

[Bug target/37010] -mno-accumulate-outgoing-args doesn't work with stack alignment

[hjl dot tools at gmail dot com]

------- Comment #5 from hjl dot tools at gmail dot com  2008-08-04 05:52 -------
It is the problem with -mno-accumulate-outgoing-args:

bash-3.2$ /export/build/gnu/gcc-avx/build-x86_64-linux/gcc/xgcc
-B/export/build/gnu/gcc-avx/build-x86_64-linux/gcc/ -m32 -msse2 -DDEBUG
-mno-accumulate-outgoing-args -mpreferred-stack-boundary=2    y.c   -o y
bash-3.2$ ./y
Segmentation fault
bash-3.2$ /export/build/gnu/gcc-avx/build-x86_64-linux/gcc/xgcc
-B/export/build/gnu/gcc-avx/build-x86_64-linux/gcc/ -m32 -msse2 -DDEBUG 
-mpreferred-stack-boundary=2    y.c   -o y
bash-3.2$ ./y
bash-3.2$ 


-- 

hjl dot tools at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|http://gcc.gnu.org/ml/gcc-  |
                   |patches/2008-               |
                   |08/msg00123.html            |
            Summary|-Os passes __m128 on stack  |-mno-accumulate-outgoing-
                   |with wrong alignment        |args doesn't work with stack
                   |                            |alignment


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

[Bug target/37010] -mno-accumulate-outgoing-args doesn't work with stack alignment

[Joey dot ye at intel dot com]

------- Comment #6 from Joey dot ye at intel dot com  2008-08-04 08:28 -------
(In reply to comment #3)
> Joey, when we compute frame layout, we don't count the duplicated
> return address pushed onto stack when DRAP is used. Also when we
> push return address, shouldn't we use -UNITS_PER_WORD, instead of
> -(STACK_BOUNDARY / BITS_PER_UNIT))? On MacOS, STACK_BOUNDARY is
> 128 on ia32. Does this patch make sense?
> --- ./i386.c.drap       2008-08-03 09:50:05.000000000 -0700
> +++ ./i386.c    2008-08-03 11:36:40.000000000 -0700
> @@ -7291,6 +7291,10 @@ ix86_compute_frame_layout (struct ix86_f
>    if (stack_realign_fp)
>      offset = (offset + stack_alignment_needed -1) & -stack_alignment_needed;
> +  /* Duplicated return address when DRAP is used.  */
> +  if (crtl->drap_reg && crtl->stack_realign_needed)
> +    offset += UNITS_PER_WORD;
> +
>    /* Register save area */
>    offset += frame->nregs * UNITS_PER_WORD;
> @@ -7692,8 +7696,7 @@ ix86_expand_prologue (void)
>          expand_builtin_return_addr etc.  */
>        x = crtl->drap_reg;
>        x = gen_frame_mem (Pmode,
> -                         plus_constant (x,
> -                                       -(STACK_BOUNDARY / BITS_PER_UNIT)));
> +                         plus_constant (x, -UNITS_PER_WORD));
>        insn = emit_insn (gen_push (x));
>        RTX_FRAME_RELATED_P (insn) = 1;
>      }
I suspect this patch is incorrect. 
  /* Skip return address and saved base pointer.  */
  offset = frame_pointer_needed ? UNITS_PER_WORD * 2 : UNITS_PER_WORD;
already count the duplicated address in. I'm analyzing what makes this case
fail.


-- 


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

[Bug target/37010] -mno-accumulate-outgoing-args doesn't work with stack alignment

[Joey dot ye at intel dot com]

------- Comment #7 from Joey dot ye at intel dot com  2008-08-04 09:03 -------
This problem is associated with -mpreferred-stack-boundary=2, rather than with
stack alignment. Following case fails on trunk before merging with stack
branch:
$ cat y1.c
/* PR middle-end/37010 */
/* { dg-do run { target { { i?86-*-* x86_64-*-* } && ilp32 } } } */
/* { dg-options "-msse2" } */

typedef __PTRDIFF_TYPE__ ptrdiff_t;
extern void abort (void);

int
__attribute__ ((noinline))
check (void *i, int align)
{
  if ((((ptrdiff_t) i) & (align - 1)) != 0)
    {
      abort ();
    }
  return 0;
}
typedef float __m128 __attribute__ ((__vector_size__ (16), __may_alias__));

void
__attribute__ ((noinline))
foo (__m128 x, __m128 y ,__m128 z ,__m128 a, int size)
{
  check(&a, __alignof__(a));
}

int
main (void)
{
  __m128 x = { 1.0 };
  foo (x, x, x, x, 5);
  return 0;
}

$ gcc -v
Using built-in specs.
Target: x86_64-unknown-linux-gnu
Configured with: ../src/configure --disable-bootstrap
--enable-languages=c,c++,fortran --enable-checking=assert
Thread model: posix
gcc version 4.4.0 20080707 (experimental) [trunk revision 137572] (GCC) 
$ gcc  -o y1.exe y1.c -m32 -Os -msse2 -mpreferred-stack-boundary=2
$ ./y.exe
Aborted


-- 


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

[Bug target/37010] -mno-accumulate-outgoing-args doesn't work with stack alignment

[Joey dot ye at intel dot com]

------- Comment #8 from Joey dot ye at intel dot com  2008-08-04 09:11 -------
Root cause is that outgoing parameter frame is aligned based on stack pointer.
Namely, address_of_stack_param = SP + offset + fixed_padding.

With -mpreferred-stack-boundary=2, alignment of SP is only 4 bytes. Outgoing
frame won't be possibly aligned with 16 bytes without additional 'and $-16,
sp'.


-- 


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

[Bug target/37010] -mno-accumulate-outgoing-args doesn't work with stack alignment

[hjl dot tools at gmail dot com]

------- Comment #9 from hjl dot tools at gmail dot com  2008-08-04 12:24 -------
(In reply to comment #7)
> This problem is associated with -mpreferred-stack-boundary=2, rather than with
> stack alignment. Following case fails on trunk before merging with stack
> branch:

Of course, before stack alignment merge, we can only align stack
variable to PREFERRED boundary, which is 1 << 2 == 4 bytes. 


-- 


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

[Bug target/37010] -mno-accumulate-outgoing-args doesn't work with stack alignment

[hjl dot tools at gmail dot com]

------- Comment #10 from hjl dot tools at gmail dot com  2008-08-04 12:26 -------
(In reply to comment #8)
> Root cause is that outgoing parameter frame is aligned based on stack pointer.
> Namely, address_of_stack_param = SP + offset + fixed_padding.
> 
> With -mpreferred-stack-boundary=2, alignment of SP is only 4 bytes. Outgoing
> frame won't be possibly aligned with 16 bytes without additional 'and $-16,
> sp'.

Did you mean we needed 2 "additional 'and $-16, sp" insns to align the
stack? I don't think so.


-- 


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

[Bug target/37010] -mno-accumulate-outgoing-args doesn't work with stack alignment

[Joey dot ye at intel dot com]

------- Comment #11 from Joey dot ye at intel dot com  2008-08-04 14:11 -------
(In reply to comment #10)
> Did you mean we needed 2 "additional 'and $-16, sp" insns to align the
> stack? I don't think so.
Definitely not. 
Solution 1: Just ignore it. __m128 parameter shouldn't be passed with
-mpreferred-stack-boundary=2, or
Solution 2. Record max alignment of all outgoing parameter, and
crtl->preferred_stack_boundary >= max_parameter_alignment


-- 


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

[Bug target/37010] -mno-accumulate-outgoing-args doesn't work with stack alignment

[hjl dot tools at gmail dot com]

------- Comment #12 from hjl dot tools at gmail dot com  2008-08-04 19:38 -------
(In reply to comment #11)
> (In reply to comment #10)
> > Did you mean we needed 2 "additional 'and $-16, sp" insns to align the
> > stack? I don't think so.
> Definitely not. 
> Solution 1: Just ignore it. __m128 parameter shouldn't be passed with
> -mpreferred-stack-boundary=2, or

That will defeat the whole purpose for stack alignment. How do you
align __m1256 when -mpreferred-stack-boundary=4?

> Solution 2. Record max alignment of all outgoing parameter, and
> crtl->preferred_stack_boundary >= max_parameter_alignment

crtl->preferred_stack_boundary is correct.

I think the problem is in

  /* Set offset to aligned because the realigned frame tarts from here.  */
  if (stack_realign_fp)
    offset = (offset + stack_alignment_needed -1) & -stack_alignment_needed;

This code assumes that offset 0 is properly aligned to any alignment,
which isn't true. It happens to work with -maccumulate-outgoing-args.


-- 


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

[Bug target/37010] -mno-accumulate-outgoing-args doesn't work with stack alignment

[hjl dot tools at gmail dot com]

------- Comment #13 from hjl dot tools at gmail dot com  2008-08-04 21:29 -------
The previous ix86_compute_frame_layout uses

  /* Align start of frame for local function.  */
  frame->padding1 = ((offset + stack_alignment_needed - 1)
                     & -stack_alignment_needed) - offset;

to align the stack offset. That was correct since before stack alignment
merge, the outgoing stack alignment is no bigger than the incoming stack
alignment. That is no longer true after stack alignment merge.


-- 


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

[Bug target/37010] -mno-accumulate-outgoing-args doesn't work with stack alignment

[hjl dot tools at gmail dot com]

------- Comment #14 from hjl dot tools at gmail dot com  2008-08-04 21:43 -------
With stack alignment, we have different stack frame layouts. We
need to handle them properly.


-- 


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

[Bug target/37010] -mno-accumulate-outgoing-args doesn't work with stack alignment

[Joey dot ye at intel dot com]

------- Comment #15 from Joey dot ye at intel dot com  2008-08-05 01:01 -------
(In reply to comment #12)
> I think the problem is in
>   /* Set offset to aligned because the realigned frame tarts from here.  */
>   if (stack_realign_fp)
>     offset = (offset + stack_alignment_needed -1) & -stack_alignment_needed;
> This code assumes that offset 0 is properly aligned to any alignment,
> which isn't true. It happens to work with -maccumulate-outgoing-args.
I still believe #8 is the right reason.


-- 


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

[Bug target/37010] -mno-accumulate-outgoing-args doesn't work with stack alignment

[hjl at gcc dot gnu dot org]

------- Comment #16 from hjl at gcc dot gnu dot org  2008-08-05 15:43 -------
Subject: Bug 37010

Author: hjl
Date: Tue Aug  5 15:41:59 2008
New Revision: 138730

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=138730
Log:
gcc/

2008-08-05  H.J. Lu  <hongjiu.lu@intel.com>

        PR middle-end/37010
        * calls.c (expand_call): Use the biggest preferred stack
        boundary.

gcc/testsuite/

2008-08-05  H.J. Lu  <hongjiu.lu@intel.com>

        PR middle-end/37010
        * gcc.dg/torture/stackalign/push-1.c: New.

Added:
    branches/stack/gcc/testsuite/gcc.dg/torture/stackalign/push-1.c
Modified:
    branches/stack/gcc/ChangeLog.stackalign
    branches/stack/gcc/calls.c
    branches/stack/gcc/testsuite/ChangeLog.stackalign


-- 


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

[Bug middle-end/37010] -mno-accumulate-outgoing-args doesn't work with stack alignment

[hjl dot tools at gmail dot com]

------- Comment #17 from hjl dot tools at gmail dot com  2008-08-05 15:43 -------
It is a middle-end bug. expand_call fails to handle increased stack
alignment when passing a parameter on stack. A patch is posted at

http://gcc.gnu.org/ml/gcc-patches/2008-08/msg00320.html


-- 

hjl dot tools at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |http://gcc.gnu.org/ml/gcc-
                   |                            |patches/2008-
                   |                            |08/msg00320.html
          Component|target                      |middle-end


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

[Bug middle-end/37010] -mno-accumulate-outgoing-args doesn't work with stack alignment

[hjl at gcc dot gnu dot org]

------- Comment #18 from hjl at gcc dot gnu dot org  2008-08-06 15:45 -------
Subject: Bug 37010

Author: hjl
Date: Wed Aug  6 15:43:46 2008
New Revision: 138808

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=138808
Log:
gcc/

2008-08-06  H.J. Lu  <hongjiu.lu@intel.com>

        PR middle-end/37010
        * calls.c (expand_call): Use the biggest preferred stack
        boundary.

gcc/testsuite/

2008-08-06  H.J. Lu  <hongjiu.lu@intel.com>

        PR middle-end/37010
        * gcc.dg/torture/stackalign/push-1.c: New.

Added:
    trunk/gcc/testsuite/gcc.dg/torture/stackalign/push-1.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/calls.c
    trunk/gcc/testsuite/ChangeLog


-- 


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

[Bug middle-end/37010] -mno-accumulate-outgoing-args doesn't work with stack alignment

[hjl dot tools at gmail dot com]

------- Comment #19 from hjl dot tools at gmail dot com  2008-08-06 15:45 -------
Fixed.


-- 

hjl dot tools at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |FIXED


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