[PATCH] D90329: [PowerPC] Fix va_arg in C++, Objective-C on 32-bit ELF targets
This revision was automatically updated to reflect the committed changes. Closed by commit rG018984ae6833: [PowerPC] Fix va_arg in C++, Objective-C on 32-bit ELF targets (authored by kernigh, committed by brad). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90329/new/ https://reviews.llvm.org/D90329 Files: clang/lib/CodeGen/TargetInfo.cpp clang/test/CodeGenCXX/ppc32-varargs-method.cpp clang/test/CodeGenObjC/ppc32-varargs-id.m Index: clang/test/CodeGenObjC/ppc32-varargs-id.m === --- /dev/null +++ clang/test/CodeGenObjC/ppc32-varargs-id.m @@ -0,0 +1,33 @@ +// REQUIRES: powerpc-registered-target +// RUN: %clang_cc1 -triple powerpc-unknown-openbsd -fblocks -emit-llvm -o - %s | FileCheck %s + +#include + +id testObject(va_list ap) { + return va_arg(ap, id); +} +// CHECK: @testObject +// CHECK: using_regs: +// CHECK-NEXT: getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %{{[0-9]+}}, i32 0, i32 4 +// CHECK-NEXT: load i8*, i8** %{{[0-9]+}}, align 4 +// CHECK-NEXT: mul i8 %numUsedRegs, 4 +// CHECK-NEXT: getelementptr inbounds i8, i8* %{{[0-9]+}}, i8 %{{[0-9]+}} +// CHECK-NEXT: bitcast i8* %{{[0-9]+}} to i8** +// CHECK-NEXT: add i8 %numUsedRegs, 1 +// CHECK-NEXT: store i8 %{{[0-9]+}}, i8* %gpr, align 4 +// CHECK-NEXT: br label %cont + +typedef void (^block)(void); +block testBlock(va_list ap) { + return va_arg(ap, block); +} +// CHECK: @testBlock +// CHECK: using_regs: +// CHECK-NEXT: getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %{{[0-9]+}}, i32 0, i32 4 +// CHECK-NEXT: load i8*, i8** %{{[0-9]+}}, align 4 +// CHECK-NEXT: mul i8 %numUsedRegs, 4 +// CHECK-NEXT: getelementptr inbounds i8, i8* %{{[0-9]+}}, i8 %{{[0-9]+}} +// CHECK-NEXT: bitcast i8* %{{[0-9]+}} to void ()** +// CHECK-NEXT: add i8 %numUsedRegs, 1 +// CHECK-NEXT: store i8 %{{[0-9]+}}, i8* %gpr, align 4 +// CHECK-NEXT: br label %cont Index: clang/test/CodeGenCXX/ppc32-varargs-method.cpp === --- /dev/null +++ clang/test/CodeGenCXX/ppc32-varargs-method.cpp @@ -0,0 +1,20 @@ +// REQUIRES: powerpc-registered-target +// RUN: %clang_cc1 -triple powerpc-unknown-openbsd -emit-llvm -o - %s | FileCheck %s + +#include + +class something; +typedef void (something::*method)(); + +method test(va_list ap) { + return va_arg(ap, method); +} +// CHECK: using_regs: +// CHECK-NEXT: getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %{{[0-9]+}}, i32 0, i32 4 +// CHECK-NEXT: load i8*, i8** %{{[0-9]+}}, align 4 +// CHECK-NEXT: mul i8 %numUsedRegs, 4 +// CHECK-NEXT: getelementptr inbounds i8, i8* %{{[0-9]+}}, i8 %{{[0-9]+}} +// CHECK-NEXT: bitcast i8* %{{[0-9]+}} to { i32, i32 }** +// CHECK-NEXT: add i8 %numUsedRegs, 1 +// CHECK-NEXT: store i8 %{{[0-9]+}}, i8* %gpr, align 4 +// CHECK-NEXT: br label %cont Index: clang/lib/CodeGen/TargetInfo.cpp === --- clang/lib/CodeGen/TargetInfo.cpp +++ clang/lib/CodeGen/TargetInfo.cpp @@ -4709,13 +4709,12 @@ // }; bool isI64 = Ty->isIntegerType() && getContext().getTypeSize(Ty) == 64; - bool isInt = - Ty->isIntegerType() || Ty->isPointerType() || Ty->isAggregateType(); + bool isInt = !Ty->isFloatingType(); bool isF64 = Ty->isFloatingType() && getContext().getTypeSize(Ty) == 64; // All aggregates are passed indirectly? That doesn't seem consistent // with the argument-lowering code. - bool isIndirect = Ty->isAggregateType(); + bool isIndirect = isAggregateTypeForABI(Ty); CGBuilderTy = CGF.Builder; Index: clang/test/CodeGenObjC/ppc32-varargs-id.m === --- /dev/null +++ clang/test/CodeGenObjC/ppc32-varargs-id.m @@ -0,0 +1,33 @@ +// REQUIRES: powerpc-registered-target +// RUN: %clang_cc1 -triple powerpc-unknown-openbsd -fblocks -emit-llvm -o - %s | FileCheck %s + +#include + +id testObject(va_list ap) { + return va_arg(ap, id); +} +// CHECK: @testObject +// CHECK: using_regs: +// CHECK-NEXT: getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %{{[0-9]+}}, i32 0, i32 4 +// CHECK-NEXT: load i8*, i8** %{{[0-9]+}}, align 4 +// CHECK-NEXT: mul i8 %numUsedRegs, 4 +// CHECK-NEXT: getelementptr inbounds i8, i8* %{{[0-9]+}}, i8 %{{[0-9]+}} +// CHECK-NEXT: bitcast i8* %{{[0-9]+}} to i8** +// CHECK-NEXT: add i8 %numUsedRegs, 1 +// CHECK-NEXT: store i8 %{{[0-9]+}}, i8* %gpr, align 4 +// CHECK-NEXT: br label %cont + +typedef void (^block)(void); +block testBlock(va_list ap) { + return va_arg(ap, block); +} +// CHECK: @testBlock +// CHECK: using_regs: +// CHECK-NEXT: getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %{{[0-9]+}}, i32 0, i32 4 +// CHECK-NEXT: load i8*, i8** %{{[0-9]+}}, align 4 +// CHECK-NEXT: mul i8 %numUsedRegs, 4 +// CHECK-NEXT: getelementptr inbounds i8, i8* %{{[0-9]+}}, i8 %{{[0-9]+}} +//
[PATCH] D90329: [PowerPC] Fix va_arg in C++, Objective-C on 32-bit ELF targets
kernigh added a comment. I forgot about this diff for a month. Comment at: clang/lib/CodeGen/TargetInfo.cpp:4722 bool isI64 = Ty->isIntegerType() && getContext().getTypeSize(Ty) == 64; - bool isInt = - Ty->isIntegerType() || Ty->isPointerType() || Ty->isAggregateType(); + bool isInt = !Ty->isFloatingType(); bool isF64 = Ty->isFloatingType() && getContext().getTypeSize(Ty) == 64; nemanjai wrote: > nit (feel free to ignore): seems like a better name might be something like > `isSingleGPR` since it would appear that this is for types that go into a > (single) general purpose register. Hi. `isInt` is also true for a 64-bit integer, which goes into a pair of GPRs, because each register has 32 bits. Comment at: clang/test/CodeGenCXX/ppc32-varargs-method.cpp:15 +// CHECK-NEXT: load i8*, i8** %{{[0-9]+}}, align 4 +// CHECK-NEXT: mul i8 %numUsedRegs, 4 +// CHECK-NEXT: getelementptr inbounds i8, i8* %{{[0-9]+}}, i8 %{{[0-9]+}} efriedma wrote: > Not sure referring to numUsedRegs like this will work in a non-Asserts build. > Please verify. I did a build with cmake -DLLVM_ENABLE_ASSERTIONS=OFF and verified that the ppc32*varargs* tests still pass. `%numUsedRegs` is still in the llvm output. `%numUsedRegs` //does not// appear in the output of `clang -S -emit-llvm ...` (not cc1), but `%numUsedRegs` //does// appear in the output of `clang -cc1 ...`. The test runs cc1 and sees the `%numUsedRegs`. I am confused by how llvm sometimes erases the name of `%numUsedRegs` and sometimes keeps the name, but I observe that turning LLVM_ENABLE_ASSERTIONS on or off doesn't affect the test. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90329/new/ https://reviews.llvm.org/D90329 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D90329: [PowerPC] Fix va_arg in C++, Objective-C on 32-bit ELF targets
nemanjai accepted this revision. nemanjai added a comment. LGTM aside from a minor nit. Comment at: clang/lib/CodeGen/TargetInfo.cpp:4722 bool isI64 = Ty->isIntegerType() && getContext().getTypeSize(Ty) == 64; - bool isInt = - Ty->isIntegerType() || Ty->isPointerType() || Ty->isAggregateType(); + bool isInt = !Ty->isFloatingType(); bool isF64 = Ty->isFloatingType() && getContext().getTypeSize(Ty) == 64; nit (feel free to ignore): seems like a better name might be something like `isSingleGPR` since it would appear that this is for types that go into a (single) general purpose register. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90329/new/ https://reviews.llvm.org/D90329 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D90329: [PowerPC] Fix va_arg in C++, Objective-C on 32-bit ELF targets
kernigh added a comment. Hi, Eli. I'm missing emails from Phabricator, so I didn't know about your recent post. I will respond to your question about numUsedRegs when I find time. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90329/new/ https://reviews.llvm.org/D90329 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D90329: [PowerPC] Fix va_arg in C++, Objective-C on 32-bit ELF targets
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM with one minor comment. Do you have commit access? If not, I can merge for you; please specify your preferred git "Author" line. Comment at: clang/test/CodeGenCXX/ppc32-varargs-method.cpp:15 +// CHECK-NEXT: load i8*, i8** %{{[0-9]+}}, align 4 +// CHECK-NEXT: mul i8 %numUsedRegs, 4 +// CHECK-NEXT: getelementptr inbounds i8, i8* %{{[0-9]+}}, i8 %{{[0-9]+}} Not sure referring to numUsedRegs like this will work in a non-Asserts build. Please verify. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90329/new/ https://reviews.llvm.org/D90329 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D90329: [PowerPC] Fix va_arg in C++, Objective-C on 32-bit ELF targets
brad added a comment. @efriedma Eli? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90329/new/ https://reviews.llvm.org/D90329 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D90329: [PowerPC] Fix va_arg in C++, Objective-C on 32-bit ELF targets
kernigh updated this revision to Diff 305335. kernigh retitled this revision from "[PowerPC] Fix va_arg in Objective-C on 32-bit ELF targets" to "[PowerPC] Fix va_arg in C++, Objective-C on 32-bit ELF targets". kernigh edited the summary of this revision. kernigh added a comment. I have updated the diff to use `bool isInt = !Ty->isFloatingType();` and `bool isIndirect = isAggregateTypeForABI(Ty);`. This removes both calls to `Ty->isAggregateType()`. This fixes some C++ types, because aren't aggregates in the C++ language, but do become aggregates in the ABI. The new test ppc32-varargs-method.cpp checks a pointer to a member function, and passes only with both the `isInt` and `isIndirect` changes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90329/new/ https://reviews.llvm.org/D90329 Files: clang/lib/CodeGen/TargetInfo.cpp clang/test/CodeGenCXX/ppc32-varargs-method.cpp clang/test/CodeGenObjC/ppc32-varargs-id.m Index: clang/test/CodeGenObjC/ppc32-varargs-id.m === --- /dev/null +++ clang/test/CodeGenObjC/ppc32-varargs-id.m @@ -0,0 +1,33 @@ +// REQUIRES: powerpc-registered-target +// RUN: %clang_cc1 -triple powerpc-unknown-openbsd -fblocks -emit-llvm -o - %s | FileCheck %s + +#include + +id testObject(va_list ap) { + return va_arg(ap, id); +} +// CHECK: @testObject +// CHECK: using_regs: +// CHECK-NEXT: getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %{{[0-9]+}}, i32 0, i32 4 +// CHECK-NEXT: load i8*, i8** %{{[0-9]+}}, align 4 +// CHECK-NEXT: mul i8 %numUsedRegs, 4 +// CHECK-NEXT: getelementptr inbounds i8, i8* %{{[0-9]+}}, i8 %{{[0-9]+}} +// CHECK-NEXT: bitcast i8* %{{[0-9]+}} to i8** +// CHECK-NEXT: add i8 %numUsedRegs, 1 +// CHECK-NEXT: store i8 %{{[0-9]+}}, i8* %gpr, align 4 +// CHECK-NEXT: br label %cont + +typedef void (^block)(void); +block testBlock(va_list ap) { + return va_arg(ap, block); +} +// CHECK: @testBlock +// CHECK: using_regs: +// CHECK-NEXT: getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %{{[0-9]+}}, i32 0, i32 4 +// CHECK-NEXT: load i8*, i8** %{{[0-9]+}}, align 4 +// CHECK-NEXT: mul i8 %numUsedRegs, 4 +// CHECK-NEXT: getelementptr inbounds i8, i8* %{{[0-9]+}}, i8 %{{[0-9]+}} +// CHECK-NEXT: bitcast i8* %{{[0-9]+}} to void ()** +// CHECK-NEXT: add i8 %numUsedRegs, 1 +// CHECK-NEXT: store i8 %{{[0-9]+}}, i8* %gpr, align 4 +// CHECK-NEXT: br label %cont Index: clang/test/CodeGenCXX/ppc32-varargs-method.cpp === --- /dev/null +++ clang/test/CodeGenCXX/ppc32-varargs-method.cpp @@ -0,0 +1,20 @@ +// REQUIRES: powerpc-registered-target +// RUN: %clang_cc1 -triple powerpc-unknown-openbsd -emit-llvm -o - %s | FileCheck %s + +#include + +class something; +typedef void (something::*method)(); + +method test(va_list ap) { + return va_arg(ap, method); +} +// CHECK: using_regs: +// CHECK-NEXT: getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %{{[0-9]+}}, i32 0, i32 4 +// CHECK-NEXT: load i8*, i8** %{{[0-9]+}}, align 4 +// CHECK-NEXT: mul i8 %numUsedRegs, 4 +// CHECK-NEXT: getelementptr inbounds i8, i8* %{{[0-9]+}}, i8 %{{[0-9]+}} +// CHECK-NEXT: bitcast i8* %{{[0-9]+}} to { i32, i32 }** +// CHECK-NEXT: add i8 %numUsedRegs, 1 +// CHECK-NEXT: store i8 %{{[0-9]+}}, i8* %gpr, align 4 +// CHECK-NEXT: br label %cont Index: clang/lib/CodeGen/TargetInfo.cpp === --- clang/lib/CodeGen/TargetInfo.cpp +++ clang/lib/CodeGen/TargetInfo.cpp @@ -4719,13 +4719,12 @@ // }; bool isI64 = Ty->isIntegerType() && getContext().getTypeSize(Ty) == 64; - bool isInt = - Ty->isIntegerType() || Ty->isPointerType() || Ty->isAggregateType(); + bool isInt = !Ty->isFloatingType(); bool isF64 = Ty->isFloatingType() && getContext().getTypeSize(Ty) == 64; // All aggregates are passed indirectly? That doesn't seem consistent // with the argument-lowering code. - bool isIndirect = Ty->isAggregateType(); + bool isIndirect = isAggregateTypeForABI(Ty); CGBuilderTy = CGF.Builder; Index: clang/test/CodeGenObjC/ppc32-varargs-id.m === --- /dev/null +++ clang/test/CodeGenObjC/ppc32-varargs-id.m @@ -0,0 +1,33 @@ +// REQUIRES: powerpc-registered-target +// RUN: %clang_cc1 -triple powerpc-unknown-openbsd -fblocks -emit-llvm -o - %s | FileCheck %s + +#include + +id testObject(va_list ap) { + return va_arg(ap, id); +} +// CHECK: @testObject +// CHECK: using_regs: +// CHECK-NEXT: getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %{{[0-9]+}}, i32 0, i32 4 +// CHECK-NEXT: load i8*, i8** %{{[0-9]+}}, align 4 +// CHECK-NEXT: mul i8 %numUsedRegs, 4 +// CHECK-NEXT: getelementptr inbounds i8, i8* %{{[0-9]+}}, i8 %{{[0-9]+}} +// CHECK-NEXT: bitcast i8* %{{[0-9]+}} to i8** +// CHECK-NEXT: add i8 %numUsedRegs, 1 +// CHECK-NEXT: store i8 %{{[0-9]+}}, i8*