[Bug target/44948] New: -mavx changes ABI

[Bug target/44948] New: -mavx changes ABI

[jakub at gcc dot gnu dot org]

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?


-- 
           Summary: -mavx changes ABI
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: jakub at gcc dot gnu dot org
GCC target triplet: x86_64-linux


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

[Bug target/44948] -mavx changes ABI

[hjl dot tools at gmail dot com]

------- Comment #1 from hjl dot tools at gmail dot com  2010-07-15 13:11 -------
How should we align

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

when it is passed on stack?


-- 


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

[Bug target/44948] -mavx changes ABI

[jakub at gcc dot gnu dot org]

------- Comment #2 from jakub at gcc dot gnu dot org  2010-07-15 13:15 -------
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.


-- 


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

[Bug target/44948] -mavx changes ABI

[hjl dot tools at gmail dot com]

------- Comment #3 from hjl dot tools at gmail dot com  2010-07-15 13:26 -------
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.


-- 

hjl dot tools at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2010-07-15 13:26:34
               date|                            |


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

[Bug target/44948] -mavx changes ABI

[hjl dot tools at gmail dot com]

------- Comment #4 from hjl dot tools at gmail dot com  2010-07-15 13:33 -------
(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.


-- 


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

[Bug target/44948] -mavx changes ABI

[hjl dot tools at gmail dot com]

------- Comment #5 from hjl dot tools at gmail dot com  2010-07-15 13:42 -------
We have aligned double to 4 byte when passing on stack
in 32bit. I guess it is OK to use a smaller alignment.


-- 


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

[Bug target/44948] -mavx changes ABI

[hjl dot tools at gmail dot com]

------- Comment #6 from hjl dot tools at gmail dot com  2010-07-15 13:56 -------
When we pass 32byte aligned type on stack with 16byte
alignment, do we mark it as 16byte aligned or 32byte
aligned?


-- 


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

[Bug target/44948] -mavx changes ABI

[hjl dot tools at gmail dot com]

------- Comment #7 from hjl dot tools at gmail dot com  2010-07-15 14:10 -------
For 32bit, we should align it to 4 byte.


-- 


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

[Bug target/44948] -mavx changes ABI

[rguenth at gcc dot gnu dot org]

------- Comment #8 from rguenth at gcc dot gnu dot org  2010-07-15 16:02 -------
(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.


-- 


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

[Bug target/44948] -mavx changes ABI

[hjl dot tools at gmail dot com]

------- Comment #9 from hjl dot tools at gmail dot com  2010-07-15 16:05 -------
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;
 }


-- 


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

[Bug target/44948] -mavx changes ABI

[hjl dot tools at gmail dot com]

------- Comment #10 from hjl dot tools at gmail dot com  2010-07-15 16:20 -------
Created an attachment (id=21216)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21216&action=view)
A patch

I am testing this patch.


-- 


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

[Bug target/44948] -mavx changes ABI

[jakub at gcc dot gnu dot org]

------- Comment #11 from jakub at gcc dot gnu dot org  2010-07-15 16:28 -------
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.


-- 


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

[Bug target/44948] -mavx changes ABI

[hjl dot tools at gmail dot com]

------- Comment #12 from hjl dot tools at gmail dot com  2010-07-15 16:41 -------
Created an attachment (id=21217)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21217&action=view)
A new patch

How about this patch?


-- 


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

[Bug target/44948] -mavx changes ABI

[hjl dot tools at gmail dot com]

------- Comment #13 from hjl dot tools at gmail dot com  2010-07-15 16:47 -------
struct A {
  long b[8] __attribute__((aligned (32)));
  __m128i x;
};

What alignment should we use to pass it on stack?


-- 


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

[Bug target/44948] -mavx changes ABI

[hjl dot tools at gmail dot com]

------- Comment #14 from hjl dot tools at gmail dot com  2010-07-15 19:07 -------
(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.


-- 


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

[Bug target/44948] -mavx changes ABI

[hjl dot tools at gmail dot com]

------- Comment #15 from hjl dot tools at gmail dot com  2010-07-15 22:29 -------
(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.


-- 


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

[Bug target/44948] -mavx changes ABI

[hjl dot tools at gmail dot com]

------- Comment #16 from hjl dot tools at gmail dot com  2010-07-15 22:47 -------
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);
  ...
}


-- 


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

[Bug target/44948] -mavx changes ABI

[hjl dot tools at gmail dot com]

------- Comment #17 from hjl dot tools at gmail dot com  2010-07-15 23:26 -------
Created an attachment (id=21220)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21220&action=view)
Another patch

This patch always aligns struct properly in 64bit
and aligns struct properly in 32bit if its alignment
is >= 16bytes.


-- 


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

[Bug target/44948] -msse/-mavx change ABI

[hjl dot tools at gmail dot com]

------- Comment #18 from hjl dot tools at gmail dot com  2010-07-16 13:31 -------
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]$ 


-- 

hjl dot tools at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|-mavx changes ABI           |-msse/-mavx change ABI


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

[Bug target/44948] -msse/-mavx change ABI

[hjl dot tools at gmail dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]



------- Comment #19 from hjl dot tools at gmail dot com  2010-07-16 13:41 -------
Created an attachment (id=21223)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=21223&action=view)
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]$ 


-- 


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

[Bug target/44948] -msse/-mavx change ABI

[hjl dot tools at gmail dot com]

------- Comment #20 from hjl dot tools at gmail dot com  2010-07-16 13:55 -------
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


-- 


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

[Bug target/44948] -msse/-mavx change ABI

[hjl dot tools at gmail dot com]

------- Comment #21 from hjl dot tools at gmail dot com  2010-08-21 17:06 -------
A patch is posted at

http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01669.html


-- 

hjl dot tools at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |http://gcc.gnu.org/ml/gcc-
                   |                            |patches/2010-
                   |                            |08/msg01669.html


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