Bug 44948 - -msse/-mavx change ABI
Summary: -msse/-mavx change ABI
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.6.0
: P3 normal
Target Milestone: 4.6.0
Assignee: Not yet assigned to anyone
URL: http://gcc.gnu.org/ml/gcc-patches/201...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-15 12:14 UTC by Jakub Jelinek
Modified: 2010-10-26 13:58 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2010-07-15 13:26:34


Attachments
A patch (660 bytes, patch)
2010-07-15 16:20 UTC, H.J. Lu
Details | Diff
A new patch (854 bytes, patch)
2010-07-15 16:41 UTC, H.J. Lu
Details | Diff
Another patch (1.07 KB, patch)
2010-07-15 23:26 UTC, H.J. Lu
Details | Diff
A patch with psABI warning (824 bytes, patch)
2010-07-16 13:41 UTC, H.J. Lu
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2010-07-15 12:14:10 UTC
With two sources:
struct A { long b[8] __attribute__((aligned (32))); };
void foo (long double, struct A);

int
main (void)
{
  struct A a = { { 0, 1, 2, 3, 4, 5, 6, 7 } };
  foo (8.0L, a);
  return 0;
}
and:
struct A { long b[8] __attribute__((aligned (32))); };

void
foo (long double x, struct A y)
{
  int i;
  if (x != 8.0L)
    __builtin_abort ();
  for (i = 0; i < 8; i++)
    if (y.b[i] != i)
      __builtin_abort ();
}

when one of these is compiled with -mavx while the other one is not, the testcase ICEs, while when -mavx is used in both or none of the compilations, it works.
This is because ix86_function_arg_boundary returns 16 for -mno-avx while 32 for -mavx in this case.  Shouldn't it return > 16 only if the mode is for 256-bit vector modes or aggregate which contains some 256-bit vector somewhere in it?
Comment 1 H.J. Lu 2010-07-15 13:11:49 UTC
How should we align

struct A { long b[8] __attribute__((aligned (32))); };

when it is passed on stack?
Comment 2 Jakub Jelinek 2010-07-15 13:15:29 UTC
If we want to be ABI compatible, I'm afraid it needs to be 16 byte aligned only.
We don't align aligned(64) structs to 64 bytes either, even with -mavx.
Comment 3 H.J. Lu 2010-07-15 13:26:34 UTC
Caller and call expander try to honor type alignment.
See PR 35771 and PR 35767. I think we should replace
BIGGEST_ALIGNMENT with MAX_STACK_ALIGNMENT.

Comment 4 H.J. Lu 2010-07-15 13:33:50 UTC
(In reply to comment #2)
> If we want to be ABI compatible, I'm afraid it needs to be 16 byte aligned
> only.
> We don't align aligned(64) structs to 64 bytes either, even with -mavx.
> 

That is because we couldn't align stack before gcc 4.4.
We may use 256bit aligned insns to copy 32byte aligned
memory. It is odd when 32byte aligned type may not be
32byte aligned when passed on stack. We may not have a
choice before gcc 4.4. I don't see why we have to do
it now. We can always issue a warning like what we did
for other ABI changes.

But I have no strong opinions on how to fix it.
Comment 5 H.J. Lu 2010-07-15 13:42:11 UTC
We have aligned double to 4 byte when passing on stack
in 32bit. I guess it is OK to use a smaller alignment.
Comment 6 H.J. Lu 2010-07-15 13:56:14 UTC
When we pass 32byte aligned type on stack with 16byte
alignment, do we mark it as 16byte aligned or 32byte
aligned?
Comment 7 H.J. Lu 2010-07-15 14:10:06 UTC
For 32bit, we should align it to 4 byte.
Comment 8 Richard Biener 2010-07-15 16:02:02 UTC
(In reply to comment #3)
> Caller and call expander try to honor type alignment.
> See PR 35771 and PR 35767. I think we should replace
> BIGGEST_ALIGNMENT with MAX_STACK_ALIGNMENT.

The call expander should not look at arguments but only at the function
prototype argument specifications.  See my comments on the patches for
the above PRs why they are wrong.
Comment 9 H.J. Lu 2010-07-15 16:05:34 UTC
How about this patch?

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 4fd2aab..65e13a3 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -6594,8 +6594,8 @@ ix86_function_arg_boundary (enum machine_mode mode, tree t
ype)
 	    align = PARM_BOUNDARY;
 	}
     }
-  if (align > BIGGEST_ALIGNMENT)
-    align = BIGGEST_ALIGNMENT;
+  else if (!contains_aligned_value_p (type))
+    align = PARM_BOUNDARY;
   return align;
 }
Comment 10 H.J. Lu 2010-07-15 16:20:41 UTC
Created attachment 21216 [details]
A patch

I am testing this patch.
Comment 11 Jakub Jelinek 2010-07-15 16:28:32 UTC
That is going to break the ABI a lot.  You'd e.g. change long double passing for -m64.
What you IMHO want is to do what the code currently does, and if the alignment after that is 32 bytes, use a predicate similar to contains_aligned_value_p
which would look for 256 bit vectors instead of 128 bit ones.
Comment 12 H.J. Lu 2010-07-15 16:41:09 UTC
Created attachment 21217 [details]
A new patch

How about this patch?
Comment 13 H.J. Lu 2010-07-15 16:47:28 UTC
struct A {
  long b[8] __attribute__((aligned (32)));
  __m128i x;
};

What alignment should we use to pass it on stack?
Comment 14 H.J. Lu 2010-07-15 19:07:18 UTC
(In reply to comment #13)
> struct A {
>   long b[8] __attribute__((aligned (32)));
>   __m128i x;
> };
> 
> What alignment should we use to pass it on stack?
> 

I think when such a struct is passed on stack, the
alignment attributes, if they are > PARM_BOUNDARY,
 are ignored when computing structure alignment.
Comment 15 H.J. Lu 2010-07-15 22:29:38 UTC
(In reply to comment #4)
> (In reply to comment #2)
> > If we want to be ABI compatible, I'm afraid it needs to be 16 byte aligned
> > only.
> > We don't align aligned(64) structs to 64 bytes either, even with -mavx.
> > 
> 
> That is because we couldn't align stack before gcc 4.4.
> We may use 256bit aligned insns to copy 32byte aligned
> memory. It is odd when 32byte aligned type may not be
> 32byte aligned when passed on stack. We may not have a
> choice before gcc 4.4. I don't see why we have to do
> it now. We can always issue a warning like what we did
> for other ABI changes.
> 
> But I have no strong opinions on how to fix it.
> 

Icc always align struct properly when passed on stack.
Comment 16 H.J. Lu 2010-07-15 22:47:22 UTC
I think we should always properly align the struct.
Otherwise, we have to deal with:

struct A { long b[8] __attribute__((aligned (32))); };

extern bar (struct A *p);

void
foo (struct A y)
{
  bar (&y);
  ...
}
Comment 17 H.J. Lu 2010-07-15 23:26:19 UTC
Created attachment 21220 [details]
Another patch

This patch always aligns struct properly in 64bit
and aligns struct properly in 32bit if its alignment
is >= 16bytes.
Comment 18 H.J. Lu 2010-07-16 13:31:46 UTC
The problem isn't new:

[hjl@gnu-6 case3]$ cat x.c
#include "x.h"

void
foo (long double x, struct A y, long double z)
{
  int i;
  struct A a = { { 0, 1, 2, 3 } };

  if (x != 8.0L || z != 8.0L)
    __builtin_abort ();
  if (__builtin_memcmp (&a, &y, sizeof (a)))
    __builtin_abort ();
}
[hjl@gnu-6 case3]$ cat x.h
struct A
{ 
  float V4SF __attribute__ ((vector_size (16)));
};

void foo (long double, struct A, long double);
[hjl@gnu-6 case3]$ cat main.c 
#include "x.h"

int
main (void)
{
  struct A a = { { 0, 1, 2, 3 } };
  foo (8.0L, a, 8.0L);
  return 0;
}
[hjl@gnu-6 case3]$ make CC=gcc
gcc -m32 -g -O -msse2   -c -o x.o x.c
gcc -m32 -g -O -mno-sse   -c -o main.o main.c
gcc -m32 -g -O -o x x.o main.o
./x
make: *** [all] Aborted
[hjl@gnu-6 case3]$ 
Comment 19 H.J. Lu 2010-07-16 13:41:51 UTC
Created attachment 21223 [details]
A patch with psABI warning

This patch changes and warns psABI:

[hjl@gnu-6 case3]$ make
/export/build/gnu/gcc/build-x86_64-linux/gcc/xgcc -B/export/build/gnu/gcc/build-x86_64-linux/gcc/ -m32 -g -O -msse2   -c -o x.o x.c
/export/build/gnu/gcc/build-x86_64-linux/gcc/xgcc -B/export/build/gnu/gcc/build-x86_64-linux/gcc/ -m32 -g -O -mno-sse   -c -o main.o main.c
main.c: In function ‘main’:
main.c:7:7: note: The ABI of passing parameter with 16byte or greater alignment has changed in GCC 4.6
/export/build/gnu/gcc/build-x86_64-linux/gcc/xgcc -B/export/build/gnu/gcc/build-x86_64-linux/gcc/ -m32 -g -O -o x x.o main.o
./x
[hjl@gnu-6 case2]$ make
/export/build/gnu/gcc/build-x86_64-linux/gcc/xgcc -B/export/build/gnu/gcc/build-x86_64-linux/gcc/ -O -mavx   -c -o x.o x.c
/export/build/gnu/gcc/build-x86_64-linux/gcc/xgcc -B/export/build/gnu/gcc/build-x86_64-linux/gcc/ -O   -c -o main.o main.c
main.c: In function ‘main’:
main.c:7:7: note: The ABI of passing parameter with 16byte or greater alignment has changed in GCC 4.6
/export/build/gnu/gcc/build-x86_64-linux/gcc/xgcc -B/export/build/gnu/gcc/build-x86_64-linux/gcc/ -O -o x x.o main.o
./x
[hjl@gnu-6 case2]$
Comment 20 H.J. Lu 2010-07-16 13:55:13 UTC
The following testcases are affected:

gcc.c-torture/compile/20070522-1.c
gcc.c-torture/compile/pr33617.c
gcc.c-torture/execute/pr38151.c
gcc.dg/compat/struct-align-1
gcc.dg/compat/struct-align-2
gcc.dg/compat/vector-1a
gcc.dg/compat/vector-1
gcc.dg/compat/vector-2a
gcc.dg/compat/vector-2b
gcc.dg/compat/vector-2
gcc.dg/pr43300.c
gcc.dg/pr44136.c
gcc.target/i386/pr39162.c
gcc.target/i386/pr40906-2.c
gcc.target/i386/sse-5.c
g++.dg/abi/param2.C
g++.dg/vect/pr33860a.cc
Comment 21 H.J. Lu 2010-08-21 17:06:38 UTC
A patch is posted at

http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01669.html
Comment 22 hjl@gcc.gnu.org 2010-10-26 13:56:46 UTC
Author: hjl
Date: Tue Oct 26 13:56:42 2010
New Revision: 165965

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=165965
Log:
Properly align parameters on stack for x86.

gcc/

2010-10-26  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/44948
	* config/i386/i386.c (ix86_old_function_arg_boundary): New.
	(ix86_function_arg_boundary): Always align parameters on stack
	in 64bit and align parameters with alignment >= 16byte on stack 
	in 32bit.  Warn alignment change.

gcc/testsuite/

2010-10-26  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/44948
	* g++.dg/abi/param2.C: Add -Wno-psabi for ilp32 x86.
	* g++.dg/vect/pr33860a.cc: Likewise.
	* gcc.c-torture/compile/20070522-1.c: Likewise.
	* gcc.dg/compat/struct-align-1_x.c: Likewise.
	* gcc.dg/compat/struct-align-1_y.c: Likewise.
	* gcc.dg/compat/struct-align-2_x.c: Likewise.
	* gcc.dg/compat/struct-align-2_y.c: Likewise.
	* gcc.dg/pr44136.c: Likewise.

	* gcc.c-torture/compile/pr33617.c: Add -Wno-psabi for x86.
	* gcc.dg/compat/vector-1_x.c: Likewise.
	* gcc.dg/compat/vector-1_y.c: Likewise.
	* gcc.dg/compat/vector-2_x.c: Likewise.
	* gcc.dg/compat/vector-2_y.c: Likewise.
	* gcc.dg/pr43300.c: Likewise.

	* gcc.dg/compat/vector-1a_x.c: Add -Wno-psabi.
	* gcc.dg/compat/vector-1a_y.c: Likewise.
	* gcc.dg/compat/vector-1b_x.c: Likewise.
	* gcc.dg/compat/vector-1b_y.c: Likewise.
	* gcc.dg/compat/vector-2a_x.c: Likewise.
	* gcc.dg/compat/vector-2a_y.c: Likewise.
	* gcc.dg/compat/vector-2b_x.c: Likewise.
	* gcc.dg/compat/vector-2b_y.c: Likewise.
	* gcc.target/i386/pr39162.c: Likewise.
	* gcc.target/i386/pr40906-2.c: Likewise.
	* gcc.target/i386/sse-5.c: Likewise.

	* gcc.dg/pr35442.c: Prune ABI change warnings.

	* gcc.c-torture/execute/pr38151.x: New.
	* gcc.target/i386/pr44948-1a.c: Likewise.
	* gcc.target/i386/pr44948-1b.c: Likewise.
	* gcc.target/i386/pr44948-2a.c: Likewise.
	* gcc.target/i386/pr44948-2b.c: Likewise.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr38151.x
    trunk/gcc/testsuite/gcc.target/i386/pr44948-1a.c
    trunk/gcc/testsuite/gcc.target/i386/pr44948-1b.c
    trunk/gcc/testsuite/gcc.target/i386/pr44948-2a.c
    trunk/gcc/testsuite/gcc.target/i386/pr44948-2b.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/abi/param2.C
    trunk/gcc/testsuite/g++.dg/vect/pr33860a.cc
    trunk/gcc/testsuite/gcc.c-torture/compile/20070522-1.c
    trunk/gcc/testsuite/gcc.c-torture/compile/pr33617.c
    trunk/gcc/testsuite/gcc.dg/compat/struct-align-1_x.c
    trunk/gcc/testsuite/gcc.dg/compat/struct-align-1_y.c
    trunk/gcc/testsuite/gcc.dg/compat/struct-align-2_x.c
    trunk/gcc/testsuite/gcc.dg/compat/struct-align-2_y.c
    trunk/gcc/testsuite/gcc.dg/compat/vector-1_x.c
    trunk/gcc/testsuite/gcc.dg/compat/vector-1_y.c
    trunk/gcc/testsuite/gcc.dg/compat/vector-1a_x.c
    trunk/gcc/testsuite/gcc.dg/compat/vector-1a_y.c
    trunk/gcc/testsuite/gcc.dg/compat/vector-1b_x.c
    trunk/gcc/testsuite/gcc.dg/compat/vector-1b_y.c
    trunk/gcc/testsuite/gcc.dg/compat/vector-2_x.c
    trunk/gcc/testsuite/gcc.dg/compat/vector-2_y.c
    trunk/gcc/testsuite/gcc.dg/compat/vector-2a_x.c
    trunk/gcc/testsuite/gcc.dg/compat/vector-2a_y.c
    trunk/gcc/testsuite/gcc.dg/compat/vector-2b_x.c
    trunk/gcc/testsuite/gcc.dg/compat/vector-2b_y.c
    trunk/gcc/testsuite/gcc.dg/pr35442.c
    trunk/gcc/testsuite/gcc.dg/pr43300.c
    trunk/gcc/testsuite/gcc.dg/pr44136.c
    trunk/gcc/testsuite/gcc.target/i386/pr39162.c
    trunk/gcc/testsuite/gcc.target/i386/pr40906-2.c
    trunk/gcc/testsuite/gcc.target/i386/sse-5.c
Comment 23 H.J. Lu 2010-10-26 13:58:57 UTC
Fixed.