[clang] 4769976 - MSVC ABI: Looks like even non-aarch64 uses the MSVC/14 definition for pod/aggregate passing
David Blaikie via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 4 13:27:37 PDT 2022
Author: David Blaikie
Date: 2022-10-04T20:19:17Z
New Revision: 4769976c49be468d7629d513080e6959a25adcfe
URL: https://github.com/llvm/llvm-project/commit/4769976c49be468d7629d513080e6959a25adcfe
DIFF: https://github.com/llvm/llvm-project/commit/4769976c49be468d7629d513080e6959a25adcfe.diff
LOG: MSVC ABI: Looks like even non-aarch64 uses the MSVC/14 definition for pod/aggregate passing
Details posted here: https://reviews.llvm.org/D119051#3747201
3 cases that were inconsistent with the MSABI without this patch applied:
https://godbolt.org/z/GY48qxh3G - field with protected member
https://godbolt.org/z/Mb1PYhjrP - non-static data member initializer
https://godbolt.org/z/sGvxcEPjo - defaulted copy constructor
I'm not sure what's suitable/sufficient testing for this - I did verify
the three cases above. Though if it helps to add them as explicit tests,
I can do that too.
Also, I was wondering if the other use of isTrivialForAArch64MSVC in
isPermittedToBeHomogenousAggregate could be another source of bugs - I
tried changing the function to unconditionally call
isTrivialFor(AArch64)MSVC without testing AArch64 first, but no tests
fail, so it looks like this is undertested in any case. But I had
trouble figuring out how to exercise this functionality properly to add
test coverage and then compare that to MSVC itself... - I got very
confused/turned around trying to test this, so I've given up enough to
send what I have out for review, but happy to look further into this
with help.
Differential Revision: https://reviews.llvm.org/D133817
Added:
Modified:
clang/lib/CodeGen/MicrosoftCXXABI.cpp
clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp
Removed:
################################################################################
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index 76aeb7bb7b76..1034066b472e 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1086,8 +1086,8 @@ bool MicrosoftCXXABI::hasMostDerivedReturn(GlobalDecl GD) const {
return isDeletingDtor(GD);
}
-static bool isTrivialForAArch64MSVC(const CXXRecordDecl *RD) {
- // For AArch64, we use the C++14 definition of an aggregate, so we also
+static bool isTrivialForMSVC(const CXXRecordDecl *RD) {
+ // We use the C++14 definition of an aggregate, so we also
// check for:
// No private or protected non static data members.
// No base classes
@@ -1115,15 +1115,7 @@ bool MicrosoftCXXABI::classifyReturnType(CGFunctionInfo &FI) const {
if (!RD)
return false;
- // Normally, the C++ concept of "is trivially copyable" is used to determine
- // if a struct can be returned directly. However, as MSVC and the language
- // have evolved, the definition of "trivially copyable" has changed, while the
- // ABI must remain stable. AArch64 uses the C++14 concept of an "aggregate",
- // while other ISAs use the older concept of "plain old data".
- bool isTrivialForABI = RD->isPOD();
- bool isAArch64 = CGM.getTarget().getTriple().isAArch64();
- if (isAArch64)
- isTrivialForABI = RD->canPassInRegisters() && isTrivialForAArch64MSVC(RD);
+ bool isTrivialForABI = RD->canPassInRegisters() && isTrivialForMSVC(RD);
// MSVC always returns structs indirectly from C++ instance methods.
bool isIndirectReturn = !isTrivialForABI || FI.isInstanceMethod();
@@ -1137,7 +1129,7 @@ bool MicrosoftCXXABI::classifyReturnType(CGFunctionInfo &FI) const {
// On AArch64, use the `inreg` attribute if the object is considered to not
// be trivially copyable, or if this is an instance method struct return.
- FI.getReturnInfo().setInReg(isAArch64);
+ FI.getReturnInfo().setInReg(CGM.getTarget().getTriple().isAArch64());
return true;
}
diff --git a/clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp b/clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp
index d6ee0684e36f..08c9faf6fd77 100644
--- a/clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp
+++ b/clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp
@@ -1,8 +1,8 @@
// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=i386-pc-linux | FileCheck -check-prefix LINUX %s
-// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=i386-pc-win32 -mconstructor-aliases -fno-rtti | FileCheck -check-prefix WIN32 %s
-// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=thumb-pc-win32 -mconstructor-aliases -fno-rtti | FileCheck -check-prefix WOA %s
-// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=x86_64-pc-win32 -mconstructor-aliases -fno-rtti | FileCheck -check-prefix WIN64 %s
-// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=aarch64-windows-msvc -mconstructor-aliases -fno-rtti | FileCheck -check-prefix WOA64 %s
+// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=i386-pc-win32 -mconstructor-aliases -fno-rtti | FileCheck -check-prefix WIN32 --check-prefix WIN %s
+// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=thumb-pc-win32 -mconstructor-aliases -fno-rtti | FileCheck -check-prefix WOA --check-prefix WIN %s
+// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=x86_64-pc-win32 -mconstructor-aliases -fno-rtti | FileCheck -check-prefix WIN64 --check-prefix WIN %s
+// RUN: %clang_cc1 -no-opaque-pointers -std=c++11 -emit-llvm %s -o - -triple=aarch64-windows-msvc -mconstructor-aliases -fno-rtti | FileCheck -check-prefix WOA64 --check-prefix WIN %s
struct Empty {};
@@ -74,6 +74,10 @@ struct SmallWithPrivate {
int i;
};
+struct SmallWithSmallWithPrivate {
+ SmallWithPrivate p;
+};
+
// WIN32: declare dso_local void @"{{.*take_bools_and_chars.*}}"
// WIN32: (<{ i8, [3 x i8], i8, [3 x i8], %struct.SmallWithDtor,
// WIN32: i8, [3 x i8], i8, [3 x i8], i32, i8, [3 x i8] }>* inalloca(<{ i8, [3 x i8], i8, [3 x i8], %struct.SmallWithDtor, i8, [3 x i8], i8, [3 x i8], i32, i8, [3 x i8] }>)
@@ -197,6 +201,10 @@ void small_arg_with_dtor(SmallWithDtor s) {}
// WOA64: define dso_local void @"?small_arg_with_private_member@@YA?AUSmallWithPrivate@@U1@@Z"(%struct.SmallWithPrivate* inreg noalias sret(%struct.SmallWithPrivate) align 4 %agg.result, i64 %s.coerce) {{.*}} {
SmallWithPrivate small_arg_with_private_member(SmallWithPrivate s) { return s; }
+// WOA64: define dso_local i32 @"?small_arg_with_small_struct_with_private_member@@YA?AUSmallWithSmallWithPrivate@@U1@@Z"(i64 %s.coerce) {{.*}} {
+// WIN64: define dso_local i32 @"?small_arg_with_small_struct_with_private_member@@YA?AUSmallWithSmallWithPrivate@@U1@@Z"(i32 %s.coerce) {{.*}} {
+SmallWithSmallWithPrivate small_arg_with_small_struct_with_private_member(SmallWithSmallWithPrivate s) { return s; }
+
void call_small_arg_with_dtor() {
small_arg_with_dtor(SmallWithDtor());
}
@@ -468,3 +476,32 @@ void C::g() { return h(SmallWithDtor()); }
// WIN64-LABEL: define dso_local void @"?g at C@pr30293@@QEAAXXZ"(%"struct.pr30293::C"* {{[^,]*}} %this)
// WIN64: declare dso_local void @"?h at C@pr30293@@UEAAXUSmallWithDtor@@@Z"(i8* noundef, i32)
}
+
+namespace protected_member_of_member {
+struct field { protected: int i; };
+struct t1 {
+ field f;
+};
+extern const t1& v1;
+t1 f1() { return v1; }
+// WIN: define dso_local {{.*}}i32 @"?f1 at protected_member_of_member@@YA?AUt1 at 1@XZ"()
+}
+
+namespace default_member_initializer {
+struct t1 {
+ int i = 3;
+};
+extern const t1& v1;
+t1 f1() { return v1; }
+// WIN: define dso_local {{.*}}i32 @"?f1 at default_member_initializer@@YA?AUt1 at 1@XZ"()
+}
+
+namespace defaulted_copy_ctor {
+struct t1 {
+ int i;
+ t1(const t1&) = default;
+};
+extern const t1& v1;
+t1 f1() { return v1; }
+// WIN: define dso_local {{.*}}i32 @"?f1 at defaulted_copy_ctor@@YA?AUt1 at 1@XZ"()
+}
More information about the cfe-commits
mailing list