Adds an option "malign-pass-aggregate" to make the alignment of the struct and union parameters compatible with the default gcc

[LiuChen3]

Summary

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 <immintrin.h>
 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 adds an option to pass struct and union following i386 System V ABI and
apply it to Linux only since other System V OS (e.g Darwin, PS4 and FreeBSD) don't
want to spend any effort dealing with the ramifications of ABI breaks at present.

[rnk]

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.

lib/CodeGen/TargetInfo.cpp

1496

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

[rjmccall]

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?

[dexonsmith]

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.

[wxiao3]

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.

[wxiao3]

Any other comments?

[dim]

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]

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

[wxiao3]

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?

[wxiao3]

Ok for merge now?

[rjmccall]

Yes, LGTM.

[krytarowski]

lib/CodeGen/TargetInfo.cpp

1498

Darwin and BSD are not System V.

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

[joerg]

lib/CodeGen/TargetInfo.cpp

1498

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.

[jyknight]

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 <immintrin.h>

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.

[pengfei]

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

[wxiao3]

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

[RKSimon]

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?

[wxiao3]

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

[emaste]

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?

[kib]

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.

[LiuChen3]

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?

[LiuChen3]

rebase.

[LiuChen3]

Ping?

[rjmccall]

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.

[LiuChen3]

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.

[rjmccall]

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.

[LiuChen3]

Add an option "malign-pass-aggregate" to compatible with gcc default passing struct and union.

[jyknight]

Since the ABI this is trying to match is not documented literally anywhere, I think we need to have some confidence that what this implements is actually the same as what GCC does. While I wrote up what I think the algorithm is, without some sort of script to allow testing it against a bunch of examples, I wouldn't say I'm confident of its correctness.

I'm not sure if you can reverse-engineer what the alignment must have been from the assembly output, or from some debug flags. Or if maybe doing something silly like modifying the source to insert a printf would be the best method to test this.

clang/lib/CodeGen/TargetInfo.cpp

1542–1544 ↗

(On Diff #250483)

This comment isn't useful. While it may be what the System V ABI document says, that's clearly incorreect, and is not what the code is or should be doing. Please document what is actually implemented, instead.

1559 ↗

(On Diff #250483)

Also needs to call getTypeStackAlignInBytes?

1567 ↗

(On Diff #250483)

I think this is wrong and that it should only return Align. The computation of the alignment of the elements is only to see if their alignment is >= 16.

If the alignment of the elements' types is >= 16, but the alignment of the structure is less than the alignment of one of its elements (e.g. due to __attribute__ packed), we should return the alignment of the structure.

1570 ↗

(On Diff #250483)

If I understood GCC's algorithm correctly, I think this needs to come first?

clang/test/CodeGen/x86_32-align-linux.cpp

9 ↗

(On Diff #250483)

Confused me that this was a different X1 than in the test-case above. I'm not sure why the tests need to be duplicated here in a .cpp file in the first place?

[rnk]

clang/include/clang/Basic/LangOptions.def

353 ↗

(On Diff #251290)

If only codegen needs to know, a CodeGenOption would be better.

clang/lib/CodeGen/TargetInfo.cpp

1556 ↗

(On Diff #251290)

Any time you crack open a record to look at the fields, the code is probably wrong the first time you write it. :( In this case, I suspect you are not looking at base classes. Consider:

struct A {
  MyAlignedType Field;
};
struct B : A {};
void passbyval(B o);

[LiuChen3]

In D60748#1931440, @jyknight wrote:

Since the ABI this is trying to match is not documented literally anywhere, I think we need to have some confidence that what this implements is actually the same as what GCC does. While I wrote up what I think the algorithm is, without some sort of script to allow testing it against a bunch of examples, I wouldn't say I'm confident of its correctness.

I'm not sure if you can reverse-engineer what the alignment must have been from the assembly output, or from some debug flags. Or if maybe doing something silly like modifying the source to insert a printf would be the best method to test this.

I think at least the initial patch is correct.

clang/include/clang/Basic/LangOptions.def

353 ↗

(On Diff #251290)

The backend does not need this option information.

clang/lib/CodeGen/TargetInfo.cpp

1556 ↗

(On Diff #251290)

I'm not sure if I understand what you mean.

typedef __attribute__((aligned(16))) int alignedint16;
typedef __attribute__((aligned(64))) int alignedint64;
struct __attribute__((aligned(64))) X2 {
  struct  __attribute__((aligned(32))) {
    int a1;
    alignedint16 a2;
  } a;
  int b;
};
struct B : X2{};
void test(B b)
{
  std::cout << b.a.a2 << std::endl;
}

This can pass.

1542–1544 ↗

(On Diff #250483)

Sorry I forget to change it.

clang/test/CodeGen/x86_32-align-linux.cpp

9 ↗

(On Diff #250483)

Sorry that I don't know much about front-end tests. I thought class, struct and union all need to be tested.

[LiuChen3]

I think at least the initial patch is correct.

I re-read your comment above, please ignore this sentence. Sorry for the noise.
My question now is that since we cannot guarantee that we are doing the right thing, is this patch necessary?

[LiuChen3]

clang/lib/CodeGen/TargetInfo.cpp

1559 ↗

(On Diff #250483)

I think this is enough.

struct __attribute__((aligned(16))) X6 {
 int x;
 struct X1 x1[5];
// alignedint64 y;
};

void test(int a, struct X6 x6)
{
  printf(%u\n", __alignof__(x6));
}

This will output 64.

1567 ↗

(On Diff #250483)

I write a test, I do n’t know if it matches your meaning.

struct __attribute__((aligned(4))) X6 {
 int x;
 alignedint64 y;
};

void test(int a, struct X6 x6)
{
  printf("%u\n", __alignof__(x6));
}

The output of gcc is 64.

If we use packed attribute:

struct __attribute__((packed)) X6 {
 int x;
 alignedint64 y;
};

Both gcc and clang with this patch output "1". And I found that the packed struct is not processed by this function.
So I think it should return the MaxAlignment .

1570 ↗

(On Diff #250483)

You mean it should be ?

if (MaxAlignment < 16)
  retrun 4
else
 return std::max(MaxAlignment, Align);

[LiuChen3]

clang/lib/CodeGen/TargetInfo.cpp

1570 ↗

(On Diff #250483)

I found that the test I wrote above was wrong. Sorry for the noise.

[RKSimon]

@wxiao3 @LiuChen3 Are you still looking at this or should it be abandoned?

[LiuChen3]

In D60748#2310148, @RKSimon wrote:

@wxiao3 @LiuChen3 Are you still looking at this or should it be abandoned?

I will abandon this patch for it is difficult to confirm the behavior of gcc.

lib/CodeGen/TargetInfo.cpp

 
 unsigned X86_32ABIInfo::getTypeStackAlignInBytes(QualType Ty,
                                                  unsigned Align) const {
   // Otherwise, if the alignment is less than or equal to the minimum ABI
   // alignment, just use the default; the backend will handle this.
   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.
-    return MinABIStackAlignInBytes;
-  }
-
-  // Otherwise, if the type contains an SSE vector type, the alignment is 16.
+  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;
+  }
+  // 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,
                                             CCState &State) const {
   if (!ByVal) {
     if (State.FreeRegs) {
       --State.FreeRegs; // Non-byval indirects just use one pointer.
       if (!IsMCUABI)

test/CodeGen/x86_32-align-linux.c

+// RUN: %clang_cc1 -w -fblocks -ffreestanding -triple i386-pc-linux-gnu -emit-llvm -o %t %s
+// RUN: FileCheck < %t %s
+
+#include <immintrin.h>
+
+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);
+}
+

test/CodeGen/x86_32-arguments-linux.c

 // RUN: %clang_cc1 -w -fblocks -triple i386-pc-linux-gnu -target-cpu pentium4 -emit-llvm -o %t %s
 // RUN: FileCheck < %t %s
 
 // 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: }
 //
 // <rdar://problem/7964854> [i386] clang misaligns long double in structures
 // when passed byval
 // <rdar://problem/8431367> clang misaligns parameters on stack
 typedef int __attribute__((vector_size (8))) t56_v2i;
 typedef double __attribute__((vector_size (8))) t56_v1d;
 typedef int __attribute__((vector_size (16))) t56_v4i;