Re: [PATCH] D21611: Fix small structures calling convention issue for some big endian architectures
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 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
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
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
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
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
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
[PATCH] D21611: Fix small structures calling convention issue for some big endian architectures
spetrovic created this revision. spetrovic added reviewers: rjmccall, john.brawn, dsanders, petarj. spetrovic added subscribers: cfe-commits, rankov, ivanbaev. Herald added a reviewer: vkalintiris. Herald added a subscriber: aemerson. This patch fixes problem with passing structures and unions smaller than register as argument in variadic functions on big endian architectures. For example passing 3 chars in a structure as argument of variadic function LLVM is passing those chars left-adjusted in register and trying to read them like they are right-adjusted, and that makes the problem. I have changed reading of those arguments. Now LLVM reads them properly (left-adjusted). I detected this problem on ARM32 big endian, MIPS32 big endian and MIPS64 big endian. http://reviews.llvm.org/D21611 Files: lib/CodeGen/TargetInfo.cpp test/CodeGen/struct-union-BE.c Index: test/CodeGen/struct-union-BE.c === --- test/CodeGen/struct-union-BE.c +++ 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: lib/CodeGen/TargetInfo.cpp === --- lib/CodeGen/TargetInfo.cpp +++ 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: test/CodeGen/struct-union-BE.c === --- test/CodeGen/struct-union-BE.c +++ 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: lib/CodeGen/TargetInfo.cpp === --- lib/CodeGen/TargetInfo.cpp +++ 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 =