Re: [PATCH] D21611: Fix small structures calling convention issue for some big endian architectures

[Strahinja Petrovic via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL273665: This patch fixes problem with passing structures and 
unions (authored by spetrovic).

Changed prior to commit:
  http://reviews.llvm.org/D21611?vs=61557=61777#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D21611

Files:
  cfe/trunk/lib/CodeGen/TargetInfo.cpp
  cfe/trunk/test/CodeGen/struct-union-BE.c

Index: cfe/trunk/test/CodeGen/struct-union-BE.c
===
--- cfe/trunk/test/CodeGen/struct-union-BE.c
+++ cfe/trunk/test/CodeGen/struct-union-BE.c
@@ -0,0 +1,48 @@
+// RUN: %clang -O2 -target mips-linux-gnu  -EB -S -emit-llvm %s -o - | 
FileCheck %s -check-prefix=MIPS
+// RUN: %clang -O2 -target mips64-linux-gnu  -EB -S -emit-llvm %s -o - | 
FileCheck %s -check-prefix=MIPS64
+// RUN: %clang -O2 -target armeb-linux-gnueabihf -march=armv7a  -EB -S 
-emit-llvm %s -o - | FileCheck %s -check-prefix=ARM
+
+#include 
+#include 
+
+struct tiny {
+  char c;
+};
+
+union data {
+  char c;
+};
+
+void fstr(int n, ...) {
+  struct tiny x;
+  va_list ap;
+  va_start (ap,n);
+  x = va_arg (ap, struct tiny);
+  if (x.c !=  10)
+abort();
+  va_end (ap);
+// MIPS-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i32 3
+// MIPS64-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i64 7
+// ARM-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i32 3
+}
+
+void funi(int n, ...) {
+  union data x;
+  va_list ap;
+  va_start (ap,n);
+  x = va_arg (ap, union data);
+  if (x.c !=  10)
+abort();
+  va_end (ap);
+// MIPS-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i32 3
+// MIPS64-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i64 7
+// ARM-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i32 3
+}
+
+void foo() {
+  struct tiny x[3];
+  union data y;
+  x[0].c = 10;
+  fstr(1, x[0]);
+  funi(1, y);
+}
Index: cfe/trunk/lib/CodeGen/TargetInfo.cpp
===
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp
@@ -272,7 +272,8 @@
 
   // If the argument is smaller than a slot, and this is a big-endian
   // target, the argument will be right-adjusted in its slot.
-  if (DirectSize < SlotSize && CGF.CGM.getDataLayout().isBigEndian()) {
+  if (DirectSize < SlotSize && CGF.CGM.getDataLayout().isBigEndian() &&
+  !DirectTy->isStructTy()) {
 Addr = CGF.Builder.CreateConstInBoundsByteGEP(Addr, SlotSize - DirectSize);
   }
 


Index: cfe/trunk/test/CodeGen/struct-union-BE.c
===
--- cfe/trunk/test/CodeGen/struct-union-BE.c
+++ cfe/trunk/test/CodeGen/struct-union-BE.c
@@ -0,0 +1,48 @@
+// RUN: %clang -O2 -target mips-linux-gnu  -EB -S -emit-llvm %s -o - | FileCheck %s -check-prefix=MIPS
+// RUN: %clang -O2 -target mips64-linux-gnu  -EB -S -emit-llvm %s -o - | FileCheck %s -check-prefix=MIPS64
+// RUN: %clang -O2 -target armeb-linux-gnueabihf -march=armv7a  -EB -S -emit-llvm %s -o - | FileCheck %s -check-prefix=ARM
+
+#include 
+#include 
+
+struct tiny {
+  char c;
+};
+
+union data {
+  char c;
+};
+
+void fstr(int n, ...) {
+  struct tiny x;
+  va_list ap;
+  va_start (ap,n);
+  x = va_arg (ap, struct tiny);
+  if (x.c !=  10)
+abort();
+  va_end (ap);
+// MIPS-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i32 3
+// MIPS64-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i64 7
+// ARM-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i32 3
+}
+
+void funi(int n, ...) {
+  union data x;
+  va_list ap;
+  va_start (ap,n);
+  x = va_arg (ap, union data);
+  if (x.c !=  10)
+abort();
+  va_end (ap);
+// MIPS-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i32 3
+// MIPS64-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i64 7
+// ARM-NOT: %{{[0-9]+}} = getelementptr inbounds i8, i8* %argp.cur, i32 3
+}
+
+void foo() {
+  struct tiny x[3];
+  union data y;
+  x[0].c = 10;
+  fstr(1, x[0]);
+  funi(1, y);
+}
Index: cfe/trunk/lib/CodeGen/TargetInfo.cpp
===
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp
@@ -272,7 +272,8 @@
 
   // If the argument is smaller than a slot, and this is a big-endian
   // target, the argument will be right-adjusted in its slot.
-  if (DirectSize < SlotSize && CGF.CGM.getDataLayout().isBigEndian()) {
+  if (DirectSize < SlotSize && CGF.CGM.getDataLayout().isBigEndian() &&
+  !DirectTy->isStructTy()) {
 Addr = CGF.Builder.CreateConstInBoundsByteGEP(Addr, SlotSize - DirectSize);
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21611: Fix small structures calling convention issue for some big endian architectures

[John Brawn via cfe-commits]

john.brawn accepted this revision.
john.brawn added a comment.

Looks OK from the ARM side as well.


http://reviews.llvm.org/D21611



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

Re: [PATCH] D21611: Fix small structures calling convention issue for some big endian architectures

[Daniel Sanders via cfe-commits]

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

In that case the MIPS side of this LGTM. Someone more familiar with ARM should 
approve it for ARM.


http://reviews.llvm.org/D21611



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

Re: [PATCH] D21611: Fix small structures calling convention issue for some big endian architectures

[Strahinja Petrovic via cfe-commits]

spetrovic added a comment.

Yes, I tried all caller - callee combinations for ARM32 big endian and 
MIPS/MIPS64 big endian, and it works properly with this patch.


http://reviews.llvm.org/D21611



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

Re: [PATCH] D21611: Fix small structures calling convention issue for some big endian architectures

[Daniel Sanders via cfe-commits]

dsanders added a comment.

This change agrees with what I think the calling convention is and the 
documentation. However, I've hit quite a few discrepancies between the 
documented calling convention and the de-facto one implemented by gcc so I'm 
wary of going by that alone.

Have you tried putting the caller and callee side of this test in different 
compilation units and then linking a gcc-compiled caller with a clang-compiled 
callee (and then repeating that for the other three combinations) like the test 
generator in tools/clang/utils/ABITest does? Does this produce a program that 
executes correctly for all four combinations?

> > Hmm.  On MIPS64, a slot is 64 bits, right?  How is a float passed?

>  Oh, floats are promoted to doubles in varargs, of course, which neatly makes 
> that an impossible situation.

> 

> My inclination is that the right condition here is that only integer types 
> should be right-justified in their slot, but I'll admit to not having an easy 
> example of a type for which your condition doesn't work.


Floats are left justified according to the 'MIPSproTM N32 ABI Handbook' (which 
also discusses the O32 and N64 ABI's) but I can't think of a test case that 
would expose a problem due to the promotion to double.



Comment at: test/CodeGen/struct-union-BE.c:1-3
@@ +1,4 @@
+// RUN: %clang -O2 -target mips-linux-gnu  -EB -S -emit-llvm %s -o - | 
FileCheck %s -check-prefix=MIPS
+// RUN: %clang -O2 -target mips64-linux-gnu  -EB -S -emit-llvm %s -o - | 
FileCheck %s -check-prefix=MIPS64
+// RUN: %clang -O2 -target armeb-linux-gnueabihf -march=armv7a  -EB -S 
-emit-llvm %s -o - | FileCheck %s -check-prefix=ARM
+

Do we need %clang and -O2? Can we use %clang_cc1 and the default optimization 
level instead?


http://reviews.llvm.org/D21611



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

Re: [PATCH] D21611: Fix small structures calling convention issue for some big endian architectures

[John McCall via cfe-commits]

rjmccall added a comment.

Oh, floats are promoted to doubles in varargs, of course, which neatly makes 
that an impossible situation.

My inclination is that the right condition here is that only integer types 
should be right-justified in their slot, but I'll admit to not having an easy 
example of a type for which your condition doesn't work.


http://reviews.llvm.org/D21611



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

Re: [PATCH] D21611: Fix small structures calling convention issue for some big endian architectures

[John McCall via cfe-commits]

rjmccall added a comment.

Hmm.  On MIPS64, a slot is 64 bits, right?  How is a float passed?


http://reviews.llvm.org/D21611



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