[PATCH] D60748: Fix i386 struct and union parameter alignment

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D60748#1925907 , @LiuChen3 wrote:

> In D60748#1924794 , @rjmccall wrote:
>
> > Oh, I see you just updated your patch months ago without ever mentioning 
> > that it was ready for review.
> >
> > It sounds to me like GCC retroactively added a switch specifying which 
> > version of the ABI to follow on this point, somewhat confusingly called 
> > `-malign-data`.  That's probably the right move here for us, too, 
> > especially since FreeBSD says they'd like to use it.  That also means the 
> > condition of when to use your new logic will have to change; basically, we 
> > need a CodeGenOption for this that will default to the old ABI, and the 
> > driver will pass down a different default on Linux.
>
>
> Thanks for review.
>
>   `-malign-data` is another topic. Just like what I said above, at least 
> `-malign-data` will not affect the calling convention of struct and union. I 
> agree with you that adding an option to control this new logi. I'll working 
> on it.


Oh, I see, sorry.  Yes, I think a different option would be good.  We can 
debate the name when the patch is ready.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[LiuChen via Phabricator via cfe-commits]

LiuChen3 added a comment.

In D60748#1924794 , @rjmccall wrote:

> Oh, I see you just updated your patch months ago without ever mentioning that 
> it was ready for review.
>
> It sounds to me like GCC retroactively added a switch specifying which 
> version of the ABI to follow on this point, somewhat confusingly called 
> `-malign-data`.  That's probably the right move here for us, too, especially 
> since FreeBSD says they'd like to use it.  That also means the condition of 
> when to use your new logic will have to change; basically, we need a 
> CodeGenOption for this that will default to the old ABI, and the driver will 
> pass down a different default on Linux.


Thanks for review.
 `-malign-data` is another topic. Just like what I said above, at least 
`-malign-data` will not affect the calling convention of struct and union. I 
agree with you that adding an option to control this new logi. I'll working on 
it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Oh, I see you just updated your patch months ago without ever mentioning that 
it was ready for review.

It sounds to me like GCC retroactively added a switch specifying which version 
of the ABI to follow on this point, somewhat confusingly called `-malign-data`. 
 That's probably the right move here for us, too, especially since FreeBSD says 
they'd like to use it.  That also means the condition of when to use your new 
logic will have to change; basically, we need a CodeGenOption for this that 
will default to the old ABI, and the driver will pass down a different default 
on Linux.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[LiuChen via Phabricator via cfe-commits]

LiuChen3 updated this revision to Diff 250483.
LiuChen3 added a comment.
Herald added a subscriber: arichardson.

rebase.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/x86_32-align-linux.c
  clang/test/CodeGen/x86_32-align-linux.cpp
  clang/test/CodeGen/x86_32-arguments-linux.c

Index: clang/test/CodeGen/x86_32-arguments-linux.c
===
--- clang/test/CodeGen/x86_32-arguments-linux.c
+++ clang/test/CodeGen/x86_32-arguments-linux.c
@@ -5,19 +5,19 @@
 // CHECK: i8 signext %a0, %struct.s56_0* byval(%struct.s56_0) align 4 %a1,
 // CHECK: i64 %a2.coerce, %struct.s56_1* byval(%struct.s56_1) align 4 %0,
 // CHECK: <1 x double> %a4, %struct.s56_2* byval(%struct.s56_2) align 4 %1,
-// CHECK: <4 x i32> %a6, %struct.s56_3* byval(%struct.s56_3) align 4 %2,
-// CHECK: <2 x double> %a8, %struct.s56_4* byval(%struct.s56_4) align 4 %3,
-// CHECK: <8 x i32> %a10, %struct.s56_5* byval(%struct.s56_5) align 4 %4,
-// CHECK: <4 x double> %a12, %struct.s56_6* byval(%struct.s56_6) align 4 %5)
+// CHECK: <4 x i32> %a6, %struct.s56_3* byval(%struct.s56_3) align 16 %a7,
+// CHECK: <2 x double> %a8, %struct.s56_4* byval(%struct.s56_4) align 16 %a9,
+// CHECK: <8 x i32> %a10, %struct.s56_5* byval(%struct.s56_5) align 32 %a11,
+// CHECK: <4 x double> %a12, %struct.s56_6* byval(%struct.s56_6) align 32 %a13)
 
 // CHECK: call void (i32, ...) @f56_0(i32 1,
 // CHECK: i32 %{{.*}}, %struct.s56_0* byval(%struct.s56_0) align 4 %{{[^ ]*}},
 // CHECK: i64 %{{[^ ]*}}, %struct.s56_1* byval(%struct.s56_1) align 4 %{{[^ ]*}},
 // CHECK: <1 x double> %{{[^ ]*}}, %struct.s56_2* byval(%struct.s56_2) align 4 %{{[^ ]*}},
-// CHECK: <4 x i32> %{{[^ ]*}}, %struct.s56_3* byval(%struct.s56_3) align 4 %{{[^ ]*}},
-// CHECK: <2 x double> %{{[^ ]*}}, %struct.s56_4* byval(%struct.s56_4) align 4 %{{[^ ]*}},
-// CHECK: <8 x i32> %{{[^ ]*}}, %struct.s56_5* byval(%struct.s56_5) align 4 %{{[^ ]*}},
-// CHECK: <4 x double> %{{[^ ]*}}, %struct.s56_6* byval(%struct.s56_6) align 4 %{{[^ ]*}})
+// CHECK: <4 x i32> %{{[^ ]*}}, %struct.s56_3* byval(%struct.s56_3) align 16 %{{[^ ]*}},
+// CHECK: <2 x double> %{{[^ ]*}}, %struct.s56_4* byval(%struct.s56_4) align 16 %{{[^ ]*}},
+// CHECK: <8 x i32> %{{[^ ]*}}, %struct.s56_5* byval(%struct.s56_5) align 32 %{{[^ ]*}},
+// CHECK: <4 x double> %{{[^ ]*}}, %struct.s56_6* byval(%struct.s56_6) align 32 %{{[^ ]*}})
 // CHECK: }
 //
 //  [i386] clang misaligns long double in structures
Index: clang/test/CodeGen/x86_32-align-linux.cpp
===
--- /dev/null
+++ clang/test/CodeGen/x86_32-align-linux.cpp
@@ -0,0 +1,75 @@
+// RUN: %clang_cc1 -w -fblocks -ffreestanding -triple i386-pc-linux-gnu -emit-llvm -o %t %s
+// RUN: FileCheck < %t %s
+
+#include 
+
+typedef __attribute__((aligned(16))) int alignedint16;
+typedef __attribute__((aligned(64))) int alignedint64;
+
+class __attribute__((aligned(64))) X1 {
+  class  __attribute__((aligned(32))) {
+   __m128 a1;
+  } a;
+  int b;
+};
+
+class __attribute__((aligned(64))) X2 {
+  class  __attribute__((aligned(32))) {
+int a1;
+alignedint16 a2;
+  } a;
+  int b;
+};
+
+class __attribute__((aligned(32))) X3 {
+  class __attribute__((aligned(64))) {
+int a1;
+alignedint16 a2;
+  } a;
+ int b;
+};
+
+class __attribute__((aligned(16))) X4 {
+  class  __attribute__((aligned(32))) {
+int a1;
+alignedint64 a2;
+  } a;
+  int b;
+};
+
+class __attribute__((aligned(64))) X5 {
+  int x;
+};
+
+class __attribute__((aligned(64))) X6 {
+  int x;
+  alignedint64 y;
+};
+
+extern void foo(int, ...);
+
+class X1 x1;
+class X2 x2;
+class X3 x3;
+class X4 x4;
+class X5 x5;
+class X6 x6;
+
+// CHECK-LABEL: define void @_Z4testv()
+// CHECK: entry:
+// CHECK: call void (i32, ...) @_Z3fooiz(i32 1, %class.X1* byval(%class.X1) align 64
+// CHECK: call void (i32, ...) @_Z3fooiz(i32 1, %class.X2* byval(%class.X2) align 64
+// CHECK: call void (i32, ...) @_Z3fooiz(i32 1, %class.X3* byval(%class.X3) align 64
+// CHECK: call void (i32, ...) @_Z3fooiz(i32 1, %class.X4* byval(%class.X4) align 64
+// CHECK: call void (i32, ...) @_Z3fooiz(i32 1, %class.X5* byval(%class.X5) align 4
+// CHECK: call void (i32, ...) @_Z3fooiz(i32 1, %class.X6* byval(%class.X6) align 64
+
+void test(void)
+{
+  foo(1, x1);
+  foo(1, x2);
+  foo(1, x3);
+  foo(1, x4);
+  foo(1, x5);
+  foo(1, x6);
+}
Index: clang/test/CodeGen/x86_32-align-linux.c
===
--- /dev/null
+++ clang/test/CodeGen/x86_32-align-linux.c
@@ -0,0 +1,96 @@
+// RUN: %clang_cc1 -w -fblocks -ffreestanding -triple i386-pc-linux-gnu -emit-llvm -o %t %s
+// RUN: FileCheck < %t %s
+
+#include 
+
+typedef union {
+  int d[4];
+   __m128 m;
+} M128;
+
+typedef __attribute__((aligned(16))) int alignedint16;
+typedef __attribute__((aligned(64))) int alignedint64;
+
+st

[PATCH] D60748: Fix i386 struct and union parameter alignment

[LiuChen via Phabricator via cfe-commits]

LiuChen3 added a comment.

Ping?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[LiuChen via Phabricator via cfe-commits]

LiuChen3 added a comment.

In D60748#1681178 , @kib wrote:

> In fact, can we have an option controlling this ?  Does it have anything to 
> do with -malign-data gcc switch ?
>
> We do want to be able to optionally generate code ABI-compatible with modern 
> gcc, per user discretion.


I found -malign-data option only affects data alignment in data segment. 
-malign-data has three options:  “compat”,“ abi” and “cacheline”.  The default 
in GCC is ”compat,“ and clang’s behavior is consistent with "abi". 
And the data alignment on stack and parameters Passing on stack is not 
affected.  This patch only affects the alignment of passing parameter.
Should we add an option just like -malign-data?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Konstantin Belousov via Phabricator via cfe-commits]

kib added a comment.

In fact, can we have an option controlling this ?  Does it have anything to do 
with -malign-data gcc switch ?

We do want to be able to optionally generate code ABI-compatible with modern 
gcc, per user discretion.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Ed Maste via Phabricator via cfe-commits]

emaste added a comment.

In D60748#1499756 , @dim wrote:

> Please also exclude FreeBSD from these changes, since we care a lot about 
> backwards compatibility, and specifically about alignment requirements.  (We 
> have run into many issues in our ports collection where upstream assumes 
> everything is 16-byte aligned on i386, which is *NOT* ABI compliant.)


@dim that said I think we'd expect to be able to mix gcc- and clang-built 
objects, and it seems this is addressing somewhat of a corner case?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[LiuChen via Phabricator via cfe-commits]

LiuChen3 updated this revision to Diff 220793.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/x86_32-align-linux.c
  clang/test/CodeGen/x86_32-align-linux.cpp
  clang/test/CodeGen/x86_32-arguments-linux.c

Index: clang/test/CodeGen/x86_32-arguments-linux.c
===
--- clang/test/CodeGen/x86_32-arguments-linux.c
+++ clang/test/CodeGen/x86_32-arguments-linux.c
@@ -5,19 +5,19 @@
 // CHECK: i8 signext %a0, %struct.s56_0* byval(%struct.s56_0) align 4 %a1,
 // CHECK: i64 %a2.coerce, %struct.s56_1* byval(%struct.s56_1) align 4 %0,
 // CHECK: <1 x double> %a4, %struct.s56_2* byval(%struct.s56_2) align 4 %1,
-// CHECK: <4 x i32> %a6, %struct.s56_3* byval(%struct.s56_3) align 4 %2,
-// CHECK: <2 x double> %a8, %struct.s56_4* byval(%struct.s56_4) align 4 %3,
-// CHECK: <8 x i32> %a10, %struct.s56_5* byval(%struct.s56_5) align 4 %4,
-// CHECK: <4 x double> %a12, %struct.s56_6* byval(%struct.s56_6) align 4 %5)
+// CHECK: <4 x i32> %a6, %struct.s56_3* byval(%struct.s56_3) align 16 %a7,
+// CHECK: <2 x double> %a8, %struct.s56_4* byval(%struct.s56_4) align 16 %a9,
+// CHECK: <8 x i32> %a10, %struct.s56_5* byval(%struct.s56_5) align 32 %a11,
+// CHECK: <4 x double> %a12, %struct.s56_6* byval(%struct.s56_6) align 32 %a13)
 
 // CHECK: call void (i32, ...) @f56_0(i32 1,
 // CHECK: i32 %{{.*}}, %struct.s56_0* byval(%struct.s56_0) align 4 %{{[^ ]*}},
 // CHECK: i64 %{{[^ ]*}}, %struct.s56_1* byval(%struct.s56_1) align 4 %{{[^ ]*}},
 // CHECK: <1 x double> %{{[^ ]*}}, %struct.s56_2* byval(%struct.s56_2) align 4 %{{[^ ]*}},
-// CHECK: <4 x i32> %{{[^ ]*}}, %struct.s56_3* byval(%struct.s56_3) align 4 %{{[^ ]*}},
-// CHECK: <2 x double> %{{[^ ]*}}, %struct.s56_4* byval(%struct.s56_4) align 4 %{{[^ ]*}},
-// CHECK: <8 x i32> %{{[^ ]*}}, %struct.s56_5* byval(%struct.s56_5) align 4 %{{[^ ]*}},
-// CHECK: <4 x double> %{{[^ ]*}}, %struct.s56_6* byval(%struct.s56_6) align 4 %{{[^ ]*}})
+// CHECK: <4 x i32> %{{[^ ]*}}, %struct.s56_3* byval(%struct.s56_3) align 16 %{{[^ ]*}},
+// CHECK: <2 x double> %{{[^ ]*}}, %struct.s56_4* byval(%struct.s56_4) align 16 %{{[^ ]*}},
+// CHECK: <8 x i32> %{{[^ ]*}}, %struct.s56_5* byval(%struct.s56_5) align 32 %{{[^ ]*}},
+// CHECK: <4 x double> %{{[^ ]*}}, %struct.s56_6* byval(%struct.s56_6) align 32 %{{[^ ]*}})
 // CHECK: }
 //
 //  [i386] clang misaligns long double in structures
Index: clang/test/CodeGen/x86_32-align-linux.cpp
===
--- /dev/null
+++ clang/test/CodeGen/x86_32-align-linux.cpp
@@ -0,0 +1,75 @@
+// RUN: %clang_cc1 -w -fblocks -ffreestanding -triple i386-pc-linux-gnu -emit-llvm -o %t %s
+// RUN: FileCheck < %t %s
+
+#include 
+
+typedef __attribute__((aligned(16))) int alignedint16;
+typedef __attribute__((aligned(64))) int alignedint64;
+
+class __attribute__((aligned(64))) X1 {
+  class  __attribute__((aligned(32))) {
+   __m128 a1;
+  } a;
+  int b;
+};
+
+class __attribute__((aligned(64))) X2 {
+  class  __attribute__((aligned(32))) {
+int a1;
+alignedint16 a2;
+  } a;
+  int b;
+};
+
+class __attribute__((aligned(32))) X3 {
+  class __attribute__((aligned(64))) {
+int a1;
+alignedint16 a2;
+  } a;
+ int b;
+};
+
+class __attribute__((aligned(16))) X4 {
+  class  __attribute__((aligned(32))) {
+int a1;
+alignedint64 a2;
+  } a;
+  int b;
+};
+
+class __attribute__((aligned(64))) X5 {
+  int x;
+};
+
+class __attribute__((aligned(64))) X6 {
+  int x;
+  alignedint64 y;
+};
+
+extern void foo(int, ...);
+
+class X1 x1;
+class X2 x2;
+class X3 x3;
+class X4 x4;
+class X5 x5;
+class X6 x6;
+
+// CHECK-LABEL: define void @_Z4testv()
+// CHECK: entry:
+// CHECK: call void (i32, ...) @_Z3fooiz(i32 1, %class.X1* byval(%class.X1) align 64
+// CHECK: call void (i32, ...) @_Z3fooiz(i32 1, %class.X2* byval(%class.X2) align 64
+// CHECK: call void (i32, ...) @_Z3fooiz(i32 1, %class.X3* byval(%class.X3) align 64
+// CHECK: call void (i32, ...) @_Z3fooiz(i32 1, %class.X4* byval(%class.X4) align 64
+// CHECK: call void (i32, ...) @_Z3fooiz(i32 1, %class.X5* byval(%class.X5) align 4
+// CHECK: call void (i32, ...) @_Z3fooiz(i32 1, %class.X6* byval(%class.X6) align 64
+
+void test(void)
+{
+  foo(1, x1);
+  foo(1, x2);
+  foo(1, x3);
+  foo(1, x4);
+  foo(1, x5);
+  foo(1, x6);
+}
Index: clang/test/CodeGen/x86_32-align-linux.c
===
--- /dev/null
+++ clang/test/CodeGen/x86_32-align-linux.c
@@ -0,0 +1,96 @@
+// RUN: %clang_cc1 -w -fblocks -ffreestanding -triple i386-pc-linux-gnu -emit-llvm -o %t %s
+// RUN: FileCheck < %t %s
+
+#include 
+
+typedef union {
+  int d[4];
+   __m128 m;
+} M128;
+
+typedef __attribute__((aligned(16))) int alignedint16;
+typedef __attribute__((aligned(64))) int alignedint64;
+
+struct __attribute__((aligned(64))) X1 {
+ struct  __attribute__((aligned(32))

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Wei Xiao via Phabricator via cfe-commits]

wxiao3 added a comment.

@RKSimon I'm busy with other stuff and my colleague: LiuChen3 will help finish 
the work.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon requested changes to this revision.
RKSimon added a comment.
This revision now requires changes to proceed.

In D60748#1524443 , @wxiao3 wrote:

> Thanks for the information!
>  We have reverted the patch and will resubmit it when we have a complete fix.


@wxiao3 Have you investigated this yet?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Wei Xiao via Phabricator via cfe-commits]

wxiao3 added a comment.

Thanks for the information!
We have reverted the patch and will resubmit it when we have a complete fix.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Pengfei Wang via Phabricator via cfe-commits]

pengfei reopened this revision.
pengfei added a comment.
This revision is now accepted and ready to land.

Reverted by https://reviews.llvm.org/rL362186


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

I don't think this was correct (where by "correct", there, I mean "what GCC 
does", as this patch is intended to match GCC behavior).

I think this change may well break more cases than it fixes, so IMO, this 
should be reverted, until it's implemented properly.

Consider one example:

  #include 
  
  typedef __attribute__((aligned(16))) int alignedint;
  
  struct __attribute__((aligned(64))) X {
  int x;
  //alignedint y;
  //__m128 y;
  };
  void g(int x, struct X);
  
  _Static_assert(_Alignof(struct X) == 64);
  
  struct X gx;
  
  void f() {
  g(1, gx);
  }

Note that when compiling this as is GCC does _not_ align X when calling g(). 
But, as of this change, now clang does. If you uncomment either the __m128 or 
alignedint lines, and now GCC aligns to 64 bytes too.

This is because GCC's algorithm is a whole lot more complex than what you've 
implemented. See its function ix86_function_arg_boundary.

The way I interpret GCC, it's doing effectively the following:
StackAlignmentForType(T):

1. If T's alignment is < 16 bytes, return 4.
2. If T is a struct/union/array type, then:
  - recursively call StackAlignmentForType() on each member's type (note -- 
this ignores any attribute((aligned(N))) directly on the //fields// of a 
struct, but not those that appear on typedefs, or the underlying types).
  - If all of those calls return alignments < 16, then return 4.
3. Otherwise, return the alignment of T.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added inline comments.



Comment at: lib/CodeGen/TargetInfo.cpp:1501
+//
+// Exclude other System V OS (e.g Darwin, PS4 and FreeBSD) since we don't
+// want to spend any effort dealing with the ramifications of ABI breaks.

krytarowski wrote:
> Darwin and BSD are not System V.
> 
> CC: @joerg @mgorny for NetBSD. Do we need to do something here?
It's a misnomer. The ABI standard for i386 was the SysV ABI before GNU decided 
to silently break the stack alignment and calling it the new ABI. That said, 
I'm not sure how much copy-by-value of vector types actually happens and that's 
the only situation affected by this.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added subscribers: mgorny, joerg.
krytarowski added inline comments.



Comment at: lib/CodeGen/TargetInfo.cpp:1501
+//
+// Exclude other System V OS (e.g Darwin, PS4 and FreeBSD) since we don't
+// want to spend any effort dealing with the ramifications of ABI breaks.

Darwin and BSD are not System V.

CC: @joerg @mgorny for NetBSD. Do we need to do something here?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Pengfei Wang via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC361934: [X86] Fix i386 struct and union parameter alignment 
(authored by pengfei, committed by ).

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748

Files:
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/x86_32-align-linux.c
  test/CodeGen/x86_32-arguments-linux.c

Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -1010,6 +1010,7 @@
   bool IsWin32StructABI;
   bool IsSoftFloatABI;
   bool IsMCUABI;
+  bool IsLinuxABI;
   unsigned DefaultNumRegisterParameters;
 
   static bool isRegisterSize(unsigned Size) {
@@ -1076,6 +1077,7 @@
   IsWin32StructABI(Win32StructABI),
   IsSoftFloatABI(SoftFloatABI),
   IsMCUABI(CGT.getTarget().getTriple().isOSIAMCU()),
+  IsLinuxABI(CGT.getTarget().getTriple().isOSLinux()),
   DefaultNumRegisterParameters(NumRegisterParameters) {}
 
   bool shouldPassIndirectlyForSwift(ArrayRef scalars,
@@ -1492,8 +1494,15 @@
   if (Align <= MinABIStackAlignInBytes)
 return 0; // Use default alignment.
 
-  // On non-Darwin, the stack type alignment is always 4.
-  if (!IsDarwinVectorABI) {
+  if (IsLinuxABI) {
+// i386 System V ABI 2.1: Structures and unions assume the alignment of their
+// most strictly aligned component.
+//
+// Exclude other System V OS (e.g Darwin, PS4 and FreeBSD) since we don't
+// want to spend any effort dealing with the ramifications of ABI breaks.
+return Align;
+  } else if (!IsDarwinVectorABI) {
+// On non-Darwin and non-Linux, the stack type alignment is always 4.
 // Set explicit alignment, since we may need to realign the top.
 return MinABIStackAlignInBytes;
   }
Index: test/CodeGen/x86_32-align-linux.c
===
--- test/CodeGen/x86_32-align-linux.c
+++ test/CodeGen/x86_32-align-linux.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -w -fblocks -ffreestanding -triple i386-pc-linux-gnu -emit-llvm -o %t %s
+// RUN: FileCheck < %t %s
+
+#include 
+
+typedef union {
+int d[4];
+__m128 m;
+} M128;
+
+extern void foo(int, ...);
+
+M128 a;
+
+// CHECK-LABEL: define void @test
+// CHECK: entry:
+// CHECK: call void (i32, ...) @foo(i32 1, %union.M128* byval align 16
+// CHECK: call void (i32, ...) @foo(i32 1, <4 x float>
+
+void test(void)
+{
+  foo(1, a);
+  foo(1, a.m);
+}
+
Index: test/CodeGen/x86_32-arguments-linux.c
===
--- test/CodeGen/x86_32-arguments-linux.c
+++ test/CodeGen/x86_32-arguments-linux.c
@@ -3,21 +3,21 @@
 
 // CHECK-LABEL: define void @f56(
 // CHECK: i8 signext %a0, %struct.s56_0* byval align 4 %a1,
-// CHECK: i64 %a2.coerce, %struct.s56_1* byval align 4,
-// CHECK: <1 x double> %a4, %struct.s56_2* byval align 4,
-// CHECK: <4 x i32> %a6, %struct.s56_3* byval align 4,
-// CHECK: <2 x double> %a8, %struct.s56_4* byval align 4,
-// CHECK: <8 x i32> %a10, %struct.s56_5* byval align 4,
-// CHECK: <4 x double> %a12, %struct.s56_6* byval align 4)
+// CHECK: i64 %a2.coerce, %struct.s56_1* byval align 8 %a3,
+// CHECK: <1 x double> %a4, %struct.s56_2* byval align 8 %a5,
+// CHECK: <4 x i32> %a6, %struct.s56_3* byval align 16 %a7,
+// CHECK: <2 x double> %a8, %struct.s56_4* byval align 16 %a9,
+// CHECK: <8 x i32> %a10, %struct.s56_5* byval align 32 %a11,
+// CHECK: <4 x double> %a12, %struct.s56_6* byval align 32 %a13)
 
 // CHECK: call void (i32, ...) @f56_0(i32 1,
 // CHECK: i32 %{{.*}}, %struct.s56_0* byval align 4 %{{[^ ]*}},
-// CHECK: i64 %{{[^ ]*}}, %struct.s56_1* byval align 4 %{{[^ ]*}},
-// CHECK: <1 x double> %{{[^ ]*}}, %struct.s56_2* byval align 4 %{{[^ ]*}},
-// CHECK: <4 x i32> %{{[^ ]*}}, %struct.s56_3* byval align 4 %{{[^ ]*}},
-// CHECK: <2 x double> %{{[^ ]*}}, %struct.s56_4* byval align 4 %{{[^ ]*}},
-// CHECK: <8 x i32> %{{[^ ]*}}, %struct.s56_5* byval align 4 %{{[^ ]*}},
-// CHECK: <4 x double> %{{[^ ]*}}, %struct.s56_6* byval align 4 %{{[^ ]*}})
+// CHECK: i64 %{{[^ ]*}}, %struct.s56_1* byval align 8 %{{[^ ]*}},
+// CHECK: <1 x double> %{{[^ ]*}}, %struct.s56_2* byval align 8 %{{[^ ]*}},
+// CHECK: <4 x i32> %{{[^ ]*}}, %struct.s56_3* byval align 16 %{{[^ ]*}},
+// CHECK: <2 x double> %{{[^ ]*}}, %struct.s56_4* byval align 16 %{{[^ ]*}},
+// CHECK: <8 x i32> %{{[^ ]*}}, %struct.s56_5* byval align 32 %{{[^ ]*}},
+// CHECK: <4 x double> %{{[^ ]*}}, %struct.s56_6* byval align 32 %{{[^ ]*}})
 // CHECK: }
 //
 //  [i386] clang misaligns long double in structures
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Yes, LGTM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Wei Xiao via Phabricator via cfe-commits]

wxiao3 added a comment.

Ok for merge now?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Wei Xiao via Phabricator via cfe-commits]

wxiao3 added a comment.

Yes, the ABI bug will cause SEGV in Linux where a lot of libraries are built by 
GCC.
I have restricted the fix to Linux only in the latest revision.

Any other comments?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Wei Xiao via Phabricator via cfe-commits]

wxiao3 updated this revision to Diff 199360.
wxiao3 edited the summary of this revision.
Herald added a subscriber: krytarowski.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748

Files:
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/x86_32-align-linux.c
  test/CodeGen/x86_32-arguments-linux.c

Index: test/CodeGen/x86_32-arguments-linux.c
===
--- test/CodeGen/x86_32-arguments-linux.c
+++ test/CodeGen/x86_32-arguments-linux.c
@@ -3,21 +3,21 @@
 
 // CHECK-LABEL: define void @f56(
 // CHECK: i8 signext %a0, %struct.s56_0* byval align 4 %a1,
-// CHECK: i64 %a2.coerce, %struct.s56_1* byval align 4,
-// CHECK: <1 x double> %a4, %struct.s56_2* byval align 4,
-// CHECK: <4 x i32> %a6, %struct.s56_3* byval align 4,
-// CHECK: <2 x double> %a8, %struct.s56_4* byval align 4,
-// CHECK: <8 x i32> %a10, %struct.s56_5* byval align 4,
-// CHECK: <4 x double> %a12, %struct.s56_6* byval align 4)
+// CHECK: i64 %a2.coerce, %struct.s56_1* byval align 8 %a3,
+// CHECK: <1 x double> %a4, %struct.s56_2* byval align 8 %a5,
+// CHECK: <4 x i32> %a6, %struct.s56_3* byval align 16 %a7,
+// CHECK: <2 x double> %a8, %struct.s56_4* byval align 16 %a9,
+// CHECK: <8 x i32> %a10, %struct.s56_5* byval align 32 %a11,
+// CHECK: <4 x double> %a12, %struct.s56_6* byval align 32 %a13)
 
 // CHECK: call void (i32, ...) @f56_0(i32 1,
 // CHECK: i32 %{{.*}}, %struct.s56_0* byval align 4 %{{[^ ]*}},
-// CHECK: i64 %{{[^ ]*}}, %struct.s56_1* byval align 4 %{{[^ ]*}},
-// CHECK: <1 x double> %{{[^ ]*}}, %struct.s56_2* byval align 4 %{{[^ ]*}},
-// CHECK: <4 x i32> %{{[^ ]*}}, %struct.s56_3* byval align 4 %{{[^ ]*}},
-// CHECK: <2 x double> %{{[^ ]*}}, %struct.s56_4* byval align 4 %{{[^ ]*}},
-// CHECK: <8 x i32> %{{[^ ]*}}, %struct.s56_5* byval align 4 %{{[^ ]*}},
-// CHECK: <4 x double> %{{[^ ]*}}, %struct.s56_6* byval align 4 %{{[^ ]*}})
+// CHECK: i64 %{{[^ ]*}}, %struct.s56_1* byval align 8 %{{[^ ]*}},
+// CHECK: <1 x double> %{{[^ ]*}}, %struct.s56_2* byval align 8 %{{[^ ]*}},
+// CHECK: <4 x i32> %{{[^ ]*}}, %struct.s56_3* byval align 16 %{{[^ ]*}},
+// CHECK: <2 x double> %{{[^ ]*}}, %struct.s56_4* byval align 16 %{{[^ ]*}},
+// CHECK: <8 x i32> %{{[^ ]*}}, %struct.s56_5* byval align 32 %{{[^ ]*}},
+// CHECK: <4 x double> %{{[^ ]*}}, %struct.s56_6* byval align 32 %{{[^ ]*}})
 // CHECK: }
 //
 //  [i386] clang misaligns long double in structures
Index: test/CodeGen/x86_32-align-linux.c
===
--- /dev/null
+++ test/CodeGen/x86_32-align-linux.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -w -fblocks -ffreestanding -triple i386-pc-linux-gnu -emit-llvm -o %t %s
+// RUN: FileCheck < %t %s
+
+#include 
+
+typedef union {
+int d[4];
+__m128 m;
+} M128;
+
+extern void foo(int, ...);
+
+M128 a;
+
+// CHECK-LABEL: define void @test
+// CHECK: entry:
+// CHECK: call void (i32, ...) @foo(i32 1, %union.M128* byval align 16
+// CHECK: call void (i32, ...) @foo(i32 1, <4 x float>
+
+void test(void)
+{
+  foo(1, a);
+  foo(1, a.m);
+}
+
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -1010,6 +1010,7 @@
   bool IsWin32StructABI;
   bool IsSoftFloatABI;
   bool IsMCUABI;
+  bool IsLinuxABI;
   unsigned DefaultNumRegisterParameters;
 
   static bool isRegisterSize(unsigned Size) {
@@ -1076,6 +1077,7 @@
   IsWin32StructABI(Win32StructABI),
   IsSoftFloatABI(SoftFloatABI),
   IsMCUABI(CGT.getTarget().getTriple().isOSIAMCU()),
+  IsLinuxABI(CGT.getTarget().getTriple().isOSLinux()),
   DefaultNumRegisterParameters(NumRegisterParameters) {}
 
   bool shouldPassIndirectlyForSwift(ArrayRef scalars,
@@ -1492,8 +1494,15 @@
   if (Align <= MinABIStackAlignInBytes)
 return 0; // Use default alignment.
 
-  // On non-Darwin, the stack type alignment is always 4.
-  if (!IsDarwinVectorABI) {
+  if (IsLinuxABI) {
+// i386 System V ABI 2.1: Structures and unions assume the alignment of their
+// most strictly aligned component.
+//
+// Exclude other System V OS (e.g Darwin, PS4 and FreeBSD) since we don't
+// want to spend any effort dealing with the ramifications of ABI breaks.
+return Align;
+  } else if (!IsDarwinVectorABI) {
+// On non-Darwin and non-Linux, the stack type alignment is always 4.
 // Set explicit alignment, since we may need to realign the top.
 return MinABIStackAlignInBytes;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Dimitry Andric via Phabricator via cfe-commits]

dim added a comment.

In fact, it is probably better to turn the OS check around, e.g. *only* 
increase the alignment for Linux, and nowhere else.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Dimitry Andric via Phabricator via cfe-commits]

dim added subscribers: emaste, dim.
dim added a comment.

Please also exclude FreeBSD from these changes, since we care a lot about 
backwards compatibility, and specifically about alignment requirements.  (We 
have run into many issues in our ports collection where upstream assumes 
everything is 16-byte aligned on i386, which is *NOT* ABI compliant.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Wei Xiao via Phabricator via cfe-commits]

wxiao3 added a comment.

Any other comments?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Wei Xiao via Phabricator via cfe-commits]

wxiao3 updated this revision to Diff 199231.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748

Files:
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/vector.c
  test/CodeGen/x86_32-arguments-darwin.c
  test/CodeGen/x86_32-arguments-linux.c
  test/CodeGen/x86_32-m64.c

Index: test/CodeGen/x86_32-m64.c
===
--- /dev/null
+++ test/CodeGen/x86_32-m64.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -w -O2 -fblocks -triple i386-pc-linux-gnu -target-cpu pentium4 -emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,LINUX
+// RUN: %clang_cc1 -w -O2 -fblocks -triple i386-apple-darwin9 -target-cpu yonah -emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,DARWIN
+// RUN: %clang_cc1 -w -O2 -fblocks -triple i386-pc-elfiamcu -mfloat-abi soft -emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,IAMCU
+// RUN: %clang_cc1 -w -O2 -fblocks -triple i386-pc-win32 -emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,WIN32
+
+#include 
+__m64 m64;
+void callee(__m64 __m1, __m64 __m2);
+__m64 caller(__m64 __m1, __m64 __m2)
+{
+// LINUX-LABEL: define x86_mmx @caller(x86_mmx %__m1.coerce, x86_mmx %__m2.coerce)
+// LINUX: tail call void @callee(x86_mmx %__m2.coerce, x86_mmx %__m1.coerce)
+// LINUX: ret x86_mmx
+// DARWIN-LABEL: define i64 @caller(i64 %__m1.coerce, i64 %__m2.coerce)
+// DARWIN: tail call void @callee(i64 %__m2.coerce, i64 %__m1.coerce)
+// DARWIN: ret i64
+// IAMCU-LABEL: define <1 x i64> @caller(i64 %__m1.coerce, i64 %__m2.coerce)
+// IAMCU: tail call void @callee(i64 %__m2.coerce, i64 %__m1.coerce)
+// IAMCU: ret <1 x i64>
+// WIN32-LABEL: define dso_local <1 x i64> @caller(i64 %__m1.coerce, i64 %__m2.coerce)
+// WIN32: call void @callee(i64 %__m2.coerce, i64 %__m1.coerce)
+// WIN32: ret <1 x i64>
+  callee(__m2, __m1);
+  return m64;
+}
Index: test/CodeGen/x86_32-arguments-linux.c
===
--- test/CodeGen/x86_32-arguments-linux.c
+++ test/CodeGen/x86_32-arguments-linux.c
@@ -3,7 +3,7 @@
 
 // CHECK-LABEL: define void @f56(
 // CHECK: i8 signext %a0, %struct.s56_0* byval align 4 %a1,
-// CHECK: i64 %a2.coerce, %struct.s56_1* byval align 4,
+// CHECK: x86_mmx %a2.coerce, %struct.s56_1* byval align 4,
 // CHECK: <1 x double> %a4, %struct.s56_2* byval align 4,
 // CHECK: <4 x i32> %a6, %struct.s56_3* byval align 4,
 // CHECK: <2 x double> %a8, %struct.s56_4* byval align 4,
@@ -12,7 +12,7 @@
 
 // CHECK: call void (i32, ...) @f56_0(i32 1,
 // CHECK: i32 %{{.*}}, %struct.s56_0* byval align 4 %{{[^ ]*}},
-// CHECK: i64 %{{[^ ]*}}, %struct.s56_1* byval align 4 %{{[^ ]*}},
+// CHECK: x86_mmx %{{[^ ]*}}, %struct.s56_1* byval align 4 %{{[^ ]*}},
 // CHECK: <1 x double> %{{[^ ]*}}, %struct.s56_2* byval align 4 %{{[^ ]*}},
 // CHECK: <4 x i32> %{{[^ ]*}}, %struct.s56_3* byval align 4 %{{[^ ]*}},
 // CHECK: <2 x double> %{{[^ ]*}}, %struct.s56_4* byval align 4 %{{[^ ]*}},
Index: test/CodeGen/x86_32-arguments-darwin.c
===
--- test/CodeGen/x86_32-arguments-darwin.c
+++ test/CodeGen/x86_32-arguments-darwin.c
@@ -229,7 +229,7 @@
 
 // CHECK-LABEL: define void @f56(
 // CHECK: i8 signext %a0, %struct.s56_0* byval align 4 %a1,
-// CHECK: i64 %a2.coerce, %struct.s56_1* byval align 4,
+// CHECK: x86_mmx %a2.coerce, %struct.s56_1* byval align 4,
 // CHECK: i64 %a4.coerce, %struct.s56_2* byval align 4,
 // CHECK: <4 x i32> %a6, %struct.s56_3* byval align 16 %a7,
 // CHECK: <2 x double> %a8, %struct.s56_4* byval align 16 %a9,
@@ -238,7 +238,7 @@
 
 // CHECK:   call void (i32, ...) @f56_0(i32 1,
 // CHECK: i32 %{{[^ ]*}}, %struct.s56_0* byval align 4 %{{[^ ]*}},
-// CHECK: i64 %{{[^ ]*}}, %struct.s56_1* byval align 4 %{{[^ ]*}},
+// CHECK: x86_mmx %{{[^ ]*}}, %struct.s56_1* byval align 4 %{{[^ ]*}},
 // CHECK: i64 %{{[^ ]*}}, %struct.s56_2* byval align 4 %{{[^ ]*}},
 // CHECK: <4 x i32> %{{[^ ]*}}, %struct.s56_3* byval align 16 %{{[^ ]*}},
 // CHECK: <2 x double> %{{[^ ]*}}, %struct.s56_4* byval align 16 %{{[^ ]*}},
Index: test/CodeGen/vector.c
===
--- test/CodeGen/vector.c
+++ test/CodeGen/vector.c
@@ -78,5 +78,5 @@
   return y;
 }
 
-// CHECK: define void @lax_vector_compare2(<2 x i32>* {{.*sret.*}}, i64 {{.*}}, i64 {{.*}})
+// CHECK: define void @lax_vector_compare2(<2 x i32>* {{.*sret.*}}, i64 {{.*}}, x86_mmx {{.*}})
 // CHECK: icmp eq <2 x i32>
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -915,14 +915,6 @@
: ABIArgInfo::getDirect());
 }
 
-/// IsX86_MMXType - Return true if this is an MMX type.
-bool IsX86_MMXType(llvm::Type *IRType) {
-  // Return true if the type is an MMX type <2 x i32>, <4 x i16>, or <8 x i8>.
-  return IRType->isVectorTy() && IRType->getPrimitiveSizeIn

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Wei Xiao via Phabricator via cfe-commits]

wxiao3 updated this revision to Diff 199232.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748

Files:
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/x86_32-align-linux.c
  test/CodeGen/x86_32-arguments-linux.c

Index: test/CodeGen/x86_32-arguments-linux.c
===
--- test/CodeGen/x86_32-arguments-linux.c
+++ test/CodeGen/x86_32-arguments-linux.c
@@ -3,21 +3,21 @@
 
 // CHECK-LABEL: define void @f56(
 // CHECK: i8 signext %a0, %struct.s56_0* byval align 4 %a1,
-// CHECK: i64 %a2.coerce, %struct.s56_1* byval align 4,
-// CHECK: <1 x double> %a4, %struct.s56_2* byval align 4,
-// CHECK: <4 x i32> %a6, %struct.s56_3* byval align 4,
-// CHECK: <2 x double> %a8, %struct.s56_4* byval align 4,
-// CHECK: <8 x i32> %a10, %struct.s56_5* byval align 4,
-// CHECK: <4 x double> %a12, %struct.s56_6* byval align 4)
+// CHECK: i64 %a2.coerce, %struct.s56_1* byval align 8 %a3,
+// CHECK: <1 x double> %a4, %struct.s56_2* byval align 8 %a5,
+// CHECK: <4 x i32> %a6, %struct.s56_3* byval align 16 %a7,
+// CHECK: <2 x double> %a8, %struct.s56_4* byval align 16 %a9,
+// CHECK: <8 x i32> %a10, %struct.s56_5* byval align 32 %a11,
+// CHECK: <4 x double> %a12, %struct.s56_6* byval align 32 %a13)
 
 // CHECK: call void (i32, ...) @f56_0(i32 1,
 // CHECK: i32 %{{.*}}, %struct.s56_0* byval align 4 %{{[^ ]*}},
-// CHECK: i64 %{{[^ ]*}}, %struct.s56_1* byval align 4 %{{[^ ]*}},
-// CHECK: <1 x double> %{{[^ ]*}}, %struct.s56_2* byval align 4 %{{[^ ]*}},
-// CHECK: <4 x i32> %{{[^ ]*}}, %struct.s56_3* byval align 4 %{{[^ ]*}},
-// CHECK: <2 x double> %{{[^ ]*}}, %struct.s56_4* byval align 4 %{{[^ ]*}},
-// CHECK: <8 x i32> %{{[^ ]*}}, %struct.s56_5* byval align 4 %{{[^ ]*}},
-// CHECK: <4 x double> %{{[^ ]*}}, %struct.s56_6* byval align 4 %{{[^ ]*}})
+// CHECK: i64 %{{[^ ]*}}, %struct.s56_1* byval align 8 %{{[^ ]*}},
+// CHECK: <1 x double> %{{[^ ]*}}, %struct.s56_2* byval align 8 %{{[^ ]*}},
+// CHECK: <4 x i32> %{{[^ ]*}}, %struct.s56_3* byval align 16 %{{[^ ]*}},
+// CHECK: <2 x double> %{{[^ ]*}}, %struct.s56_4* byval align 16 %{{[^ ]*}},
+// CHECK: <8 x i32> %{{[^ ]*}}, %struct.s56_5* byval align 32 %{{[^ ]*}},
+// CHECK: <4 x double> %{{[^ ]*}}, %struct.s56_6* byval align 32 %{{[^ ]*}})
 // CHECK: }
 //
 //  [i386] clang misaligns long double in structures
Index: test/CodeGen/x86_32-align-linux.c
===
--- /dev/null
+++ test/CodeGen/x86_32-align-linux.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -w -fblocks -ffreestanding -triple i386-pc-linux-gnu -emit-llvm -o %t %s
+// RUN: FileCheck < %t %s
+
+#include 
+
+typedef union {
+int d[4];
+__m128 m;
+} M128;
+
+extern void foo(int, ...);
+
+M128 a;
+
+// CHECK-LABEL: define void @test
+// CHECK: entry:
+// CHECK: call void (i32, ...) @foo(i32 1, %union.M128* byval align 16
+// CHECK: call void (i32, ...) @foo(i32 1, <4 x float>
+
+void test(void)
+{
+  foo(1, a);
+  foo(1, a.m);
+}
+
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -1010,6 +1010,7 @@
   bool IsWin32StructABI;
   bool IsSoftFloatABI;
   bool IsMCUABI;
+  bool IsPS4ABI;
   unsigned DefaultNumRegisterParameters;
 
   static bool isRegisterSize(unsigned Size) {
@@ -1076,6 +1077,7 @@
   IsWin32StructABI(Win32StructABI),
   IsSoftFloatABI(SoftFloatABI),
   IsMCUABI(CGT.getTarget().getTriple().isOSIAMCU()),
+  IsPS4ABI(CGT.getTarget().getTriple().isPS4()),
   DefaultNumRegisterParameters(NumRegisterParameters) {}
 
   bool shouldPassIndirectlyForSwift(ArrayRef scalars,
@@ -1492,18 +1494,18 @@
   if (Align <= MinABIStackAlignInBytes)
 return 0; // Use default alignment.
 
-  // On non-Darwin, the stack type alignment is always 4.
-  if (!IsDarwinVectorABI) {
-// Set explicit alignment, since we may need to realign the top.
+  if (IsDarwinVectorABI) {
+// On Darwin, if the type contains an SSE vector type, the alignment is 16.
+if (Align >= 16 && (isSSEVectorType(getContext(), Ty) ||
+  isRecordWithSSEVectorType(getContext(), Ty)))
+  return 16;
+return MinABIStackAlignInBytes;
+  } else if (IsWin32StructABI || IsPS4ABI) {
 return MinABIStackAlignInBytes;
   }
-
-  // Otherwise, if the type contains an SSE vector type, the alignment is 16.
-  if (Align >= 16 && (isSSEVectorType(getContext(), Ty) ||
-  isRecordWithSSEVectorType(getContext(), Ty)))
-return 16;
-
-  return MinABIStackAlignInBytes;
+  // i386 System V ABI 2.1: Structures and unions assume the alignment of their
+  // most strictly aligned component.
+  return Align;
 }
 
 ABIArgInfo X86_32ABIInfo::getIndirectResult(QualType Ty, bool ByVal,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listi

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Wei Xiao via Phabricator via cfe-commits]

wxiao3 added a comment.

Ok, I have excluded Darwin and PS4 for the changes.
The fix mainly targets at Linux so that we can compile a project with parts by 
GCC and parts by LLVM given that they follow the same ABI.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Wei Xiao via Phabricator via cfe-commits]

wxiao3 updated this revision to Diff 196967.
wxiao3 edited the summary of this revision.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748

Files:
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/x86_32-align-linux.c
  test/CodeGen/x86_32-arguments-linux.c

Index: test/CodeGen/x86_32-arguments-linux.c
===
--- test/CodeGen/x86_32-arguments-linux.c
+++ test/CodeGen/x86_32-arguments-linux.c
@@ -3,21 +3,21 @@
 
 // CHECK-LABEL: define void @f56(
 // CHECK: i8 signext %a0, %struct.s56_0* byval align 4 %a1,
-// CHECK: i64 %a2.coerce, %struct.s56_1* byval align 4,
-// CHECK: <1 x double> %a4, %struct.s56_2* byval align 4,
-// CHECK: <4 x i32> %a6, %struct.s56_3* byval align 4,
-// CHECK: <2 x double> %a8, %struct.s56_4* byval align 4,
-// CHECK: <8 x i32> %a10, %struct.s56_5* byval align 4,
-// CHECK: <4 x double> %a12, %struct.s56_6* byval align 4)
+// CHECK: i64 %a2.coerce, %struct.s56_1* byval align 8 %a3,
+// CHECK: <1 x double> %a4, %struct.s56_2* byval align 8 %a5,
+// CHECK: <4 x i32> %a6, %struct.s56_3* byval align 16 %a7,
+// CHECK: <2 x double> %a8, %struct.s56_4* byval align 16 %a9,
+// CHECK: <8 x i32> %a10, %struct.s56_5* byval align 32 %a11,
+// CHECK: <4 x double> %a12, %struct.s56_6* byval align 32 %a13)
 
 // CHECK: call void (i32, ...) @f56_0(i32 1,
 // CHECK: i32 %{{.*}}, %struct.s56_0* byval align 4 %{{[^ ]*}},
-// CHECK: i64 %{{[^ ]*}}, %struct.s56_1* byval align 4 %{{[^ ]*}},
-// CHECK: <1 x double> %{{[^ ]*}}, %struct.s56_2* byval align 4 %{{[^ ]*}},
-// CHECK: <4 x i32> %{{[^ ]*}}, %struct.s56_3* byval align 4 %{{[^ ]*}},
-// CHECK: <2 x double> %{{[^ ]*}}, %struct.s56_4* byval align 4 %{{[^ ]*}},
-// CHECK: <8 x i32> %{{[^ ]*}}, %struct.s56_5* byval align 4 %{{[^ ]*}},
-// CHECK: <4 x double> %{{[^ ]*}}, %struct.s56_6* byval align 4 %{{[^ ]*}})
+// CHECK: i64 %{{[^ ]*}}, %struct.s56_1* byval align 8 %{{[^ ]*}},
+// CHECK: <1 x double> %{{[^ ]*}}, %struct.s56_2* byval align 8 %{{[^ ]*}},
+// CHECK: <4 x i32> %{{[^ ]*}}, %struct.s56_3* byval align 16 %{{[^ ]*}},
+// CHECK: <2 x double> %{{[^ ]*}}, %struct.s56_4* byval align 16 %{{[^ ]*}},
+// CHECK: <8 x i32> %{{[^ ]*}}, %struct.s56_5* byval align 32 %{{[^ ]*}},
+// CHECK: <4 x double> %{{[^ ]*}}, %struct.s56_6* byval align 32 %{{[^ ]*}})
 // CHECK: }
 //
 //  [i386] clang misaligns long double in structures
Index: test/CodeGen/x86_32-align-linux.c
===
--- /dev/null
+++ test/CodeGen/x86_32-align-linux.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -w -fblocks -ffreestanding -triple i386-pc-linux-gnu -emit-llvm -o %t %s
+// RUN: FileCheck < %t %s
+
+#include 
+
+typedef union {
+int d[4];
+__m128 m;
+} M128;
+
+extern void foo(int, ...);
+
+M128 a;
+
+// CHECK-LABEL: define void @test
+// CHECK: entry:
+// CHECK: call void (i32, ...) @foo(i32 1, %union.M128* byval align 16
+// CHECK: call void (i32, ...) @foo(i32 1, <4 x float>
+
+void test(void)
+{
+  foo(1, a);
+  foo(1, a.m);
+}
+
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -1010,6 +1010,7 @@
   bool IsWin32StructABI;
   bool IsSoftFloatABI;
   bool IsMCUABI;
+  bool IsPS4ABI;
   unsigned DefaultNumRegisterParameters;
 
   static bool isRegisterSize(unsigned Size) {
@@ -1076,6 +1077,7 @@
   IsWin32StructABI(Win32StructABI),
   IsSoftFloatABI(SoftFloatABI),
   IsMCUABI(CGT.getTarget().getTriple().isOSIAMCU()),
+  IsPS4ABI(CGT.getTarget().getTriple().isPS4()),
   DefaultNumRegisterParameters(NumRegisterParameters) {}
 
   bool shouldPassIndirectlyForSwift(ArrayRef scalars,
@@ -1492,18 +1494,18 @@
   if (Align <= MinABIStackAlignInBytes)
 return 0; // Use default alignment.
 
-  // On non-Darwin, the stack type alignment is always 4.
-  if (!IsDarwinVectorABI) {
-// Set explicit alignment, since we may need to realign the top.
+  if (IsDarwinVectorABI) {
+// On Darwin, if the type contains an SSE vector type, the alignment is 16.
+if (Align >= 16 && (isSSEVectorType(getContext(), Ty) ||
+  isRecordWithSSEVectorType(getContext(), Ty)))
+  return 16;
+return MinABIStackAlignInBytes;
+  } else if (IsWin32StructABI || IsPS4ABI) {
 return MinABIStackAlignInBytes;
   }
-
-  // Otherwise, if the type contains an SSE vector type, the alignment is 16.
-  if (Align >= 16 && (isSSEVectorType(getContext(), Ty) ||
-  isRecordWithSSEVectorType(getContext(), Ty)))
-return 16;
-
-  return MinABIStackAlignInBytes;
+  // i386 System V ABI 2.1: Structures and unions assume the alignment of their
+  // most strictly aligned component.
+  return Align;
 }
 
 ABIArgInfo X86_32ABIInfo::getIndirectResult(QualType Ty, bool ByVal,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D60748#1472829 , @rjmccall wrote:

> I suspect Darwin also doesn't want to take this.  We care very little about 
> 32-bit Intel, and part of caring very little is not wanting to spend any 
> effort dealing with the ramifications of ABI breaks.  That would apply both 
> to the rule in general and to the vector rule specifically.  @dexonsmith, 
> agreed?


Agreed.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[John McCall via Phabricator via cfe-commits]

rjmccall added a subscriber: dexonsmith.
rjmccall added a comment.

I suspect Darwin also doesn't want to take this.  We care very little about 
32-bit Intel, and part of caring very little is not wanting to spend any effort 
dealing with the ramifications of ABI breaks.  That would apply both to the 
rule in general and to the vector rule specifically.  @dexonsmith, agreed?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

This is, obviously, an ABI break. I think Sony would probably want you to 
preserve the existing behavior of intentionally underaligning such byval 
parameters for PS4 targets. +@rjmccall in case he has other ABI thoughts.




Comment at: lib/CodeGen/TargetInfo.cpp:1496
+  if (IsDarwinVectorABI) {
+// On Darwin, if the type contains an SSE vector type, the alignment is 16.
+if (Align >= 16 && (isSSEVectorType(getContext(), Ty) ||

@rjmccall, does this comment need updating in an AVX world?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60748/new/

https://reviews.llvm.org/D60748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60748: Fix i386 struct and union parameter alignment

[Wei Xiao via Phabricator via cfe-commits]

wxiao3 created this revision.
wxiao3 added reviewers: annita.zhang, LuoYuanke, smaslov, hjl.tools, RKSimon, 
rnk, andreadb.
wxiao3 added a project: clang.
Herald added a subscriber: cfe-commits.

According to i386 System V ABI 2.1: Structures and unions assume the alignment
of their most strictly aligned component. But current implementation always
takes them as 4-byte aligned which will result in incorrect code, e.g:

  1 #include 
  2 typedef union {
  3 int d[4];
  4 __m128 m;
  5 } M128;
  6 extern void foo(int, ...);
  7 void test(void)
  8 {
  9   M128 a;

10   foo(1, a);
 11   foo(1, a.m);
 12 }

The first call (line 10) takes the second arg as 4-byte aligned while the
second call (line 11) takes the second arg as 16-byte aligned. There is
oxymoron for the alignment of the 2 calls because they should be the same.

This patch fixes the bug by following i386 System V ABI.


Repository:
  rC Clang

https://reviews.llvm.org/D60748

Files:
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/x86_32-align-linux.c
  test/CodeGen/x86_32-arguments-linux.c

Index: test/CodeGen/x86_32-arguments-linux.c
===
--- test/CodeGen/x86_32-arguments-linux.c
+++ test/CodeGen/x86_32-arguments-linux.c
@@ -3,21 +3,21 @@
 
 // CHECK-LABEL: define void @f56(
 // CHECK: i8 signext %a0, %struct.s56_0* byval align 4 %a1,
-// CHECK: i64 %a2.coerce, %struct.s56_1* byval align 4,
-// CHECK: <1 x double> %a4, %struct.s56_2* byval align 4,
-// CHECK: <4 x i32> %a6, %struct.s56_3* byval align 4,
-// CHECK: <2 x double> %a8, %struct.s56_4* byval align 4,
-// CHECK: <8 x i32> %a10, %struct.s56_5* byval align 4,
-// CHECK: <4 x double> %a12, %struct.s56_6* byval align 4)
+// CHECK: i64 %a2.coerce, %struct.s56_1* byval align 8 %a3,
+// CHECK: <1 x double> %a4, %struct.s56_2* byval align 8 %a5,
+// CHECK: <4 x i32> %a6, %struct.s56_3* byval align 16 %a7,
+// CHECK: <2 x double> %a8, %struct.s56_4* byval align 16 %a9,
+// CHECK: <8 x i32> %a10, %struct.s56_5* byval align 32 %a11,
+// CHECK: <4 x double> %a12, %struct.s56_6* byval align 32 %a13)
 
 // CHECK: call void (i32, ...) @f56_0(i32 1,
 // CHECK: i32 %{{.*}}, %struct.s56_0* byval align 4 %{{[^ ]*}},
-// CHECK: i64 %{{[^ ]*}}, %struct.s56_1* byval align 4 %{{[^ ]*}},
-// CHECK: <1 x double> %{{[^ ]*}}, %struct.s56_2* byval align 4 %{{[^ ]*}},
-// CHECK: <4 x i32> %{{[^ ]*}}, %struct.s56_3* byval align 4 %{{[^ ]*}},
-// CHECK: <2 x double> %{{[^ ]*}}, %struct.s56_4* byval align 4 %{{[^ ]*}},
-// CHECK: <8 x i32> %{{[^ ]*}}, %struct.s56_5* byval align 4 %{{[^ ]*}},
-// CHECK: <4 x double> %{{[^ ]*}}, %struct.s56_6* byval align 4 %{{[^ ]*}})
+// CHECK: i64 %{{[^ ]*}}, %struct.s56_1* byval align 8 %{{[^ ]*}},
+// CHECK: <1 x double> %{{[^ ]*}}, %struct.s56_2* byval align 8 %{{[^ ]*}},
+// CHECK: <4 x i32> %{{[^ ]*}}, %struct.s56_3* byval align 16 %{{[^ ]*}},
+// CHECK: <2 x double> %{{[^ ]*}}, %struct.s56_4* byval align 16 %{{[^ ]*}},
+// CHECK: <8 x i32> %{{[^ ]*}}, %struct.s56_5* byval align 32 %{{[^ ]*}},
+// CHECK: <4 x double> %{{[^ ]*}}, %struct.s56_6* byval align 32 %{{[^ ]*}})
 // CHECK: }
 //
 //  [i386] clang misaligns long double in structures
Index: test/CodeGen/x86_32-align-linux.c
===
--- /dev/null
+++ test/CodeGen/x86_32-align-linux.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -w -fblocks -ffreestanding -triple i386-pc-linux-gnu -emit-llvm -o %t %s
+// RUN: FileCheck < %t %s
+
+#include 
+
+typedef union {
+int d[4];
+__m128 m;
+} M128;
+
+extern void foo(int, ...);
+
+M128 a;
+
+// CHECK-LABEL: define void @test
+// CHECK: entry:
+// CHECK: call void (i32, ...) @foo(i32 1, %union.M128* byval align 16
+// CHECK: call void (i32, ...) @foo(i32 1, <4 x float>
+
+void test(void)
+{
+  foo(1, a);
+  foo(1, a.m);
+}
+
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -1492,18 +1492,18 @@
   if (Align <= MinABIStackAlignInBytes)
 return 0; // Use default alignment.
 
-  // On non-Darwin, the stack type alignment is always 4.
-  if (!IsDarwinVectorABI) {
-// Set explicit alignment, since we may need to realign the top.
+  if (IsDarwinVectorABI) {
+// On Darwin, if the type contains an SSE vector type, the alignment is 16.
+if (Align >= 16 && (isSSEVectorType(getContext(), Ty) ||
+  isRecordWithSSEVectorType(getContext(), Ty)))
+  return 16;
+return MinABIStackAlignInBytes;
+  } else if (IsWin32StructABI) {
 return MinABIStackAlignInBytes;
   }
-
-  // Otherwise, if the type contains an SSE vector type, the alignment is 16.
-  if (Align >= 16 && (isSSEVectorType(getContext(), Ty) ||
-  isRecordWithSSEVectorType(getContext(), Ty)))
-return 16;
-
-  return MinABIStackAlignInBytes;
+  // i386 System V ABI 2.1: Structures and unions assume the alignment of the