Author: Alex Bradbury Date: 2023-07-24T10:24:34+01:00 New Revision: 17a58b3ca7ec18585e9ea8ed8b39d72fe36fb6cb
URL: https://github.com/llvm/llvm-project/commit/17a58b3ca7ec18585e9ea8ed8b39d72fe36fb6cb DIFF: https://github.com/llvm/llvm-project/commit/17a58b3ca7ec18585e9ea8ed8b39d72fe36fb6cb.diff LOG: [clang][RISCV] Fix ABI handling of empty structs with hard FP calling conventions in C++ As reported in <https://github.com/llvm/llvm-project/issues/58929>, Clang's handling of empty structs in the case of small structs that may be eligible to be passed using the hard FP calling convention doesn't match g++. In general, C++ record fields are never empty unless [[no_unique_address]] is used, but the RISC-V FP ABI overrides this. After this patch, fields of structs that contain empty records will be ignored, even in C++, when considering eligibility for the FP calling convention ('flattening'). See also the relevant psABI issue <https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/358> which seeks to clarify the documentation. Fixes https://github.com/llvm/llvm-project/issues/58929 Differential Revision: https://reviews.llvm.org/D142327 Added: Modified: clang/docs/ReleaseNotes.rst clang/lib/CodeGen/ABIInfoImpl.cpp clang/lib/CodeGen/ABIInfoImpl.h clang/lib/CodeGen/Targets/RISCV.cpp clang/test/CodeGen/RISCV/abi-empty-structs.c Removed: ################################################################################ diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index db9149fae797c4..1df474d2672720 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -899,6 +899,9 @@ RISC-V Support - The rules for ordering of extensions in ``-march`` strings were relaxed. A canonical ordering is no longer enforced on ``z*``, ``s*``, and ``x*`` prefixed extensions. +* An ABI mismatch between GCC and Clang related to the handling of empty + structs in C++ parameter passing under the hard floating point calling + conventions was fixed. CUDA/HIP Language Changes ^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/clang/lib/CodeGen/ABIInfoImpl.cpp b/clang/lib/CodeGen/ABIInfoImpl.cpp index 7c30cecfdb9b77..2b20d5a13346d3 100644 --- a/clang/lib/CodeGen/ABIInfoImpl.cpp +++ b/clang/lib/CodeGen/ABIInfoImpl.cpp @@ -246,7 +246,7 @@ Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address Addr1, } bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD, - bool AllowArrays) { + bool AllowArrays, bool AsIfNoUniqueAddr) { if (FD->isUnnamedBitfield()) return true; @@ -280,13 +280,14 @@ bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD, // not arrays of records, so we must also check whether we stripped off an // array type above. if (isa<CXXRecordDecl>(RT->getDecl()) && - (WasArray || !FD->hasAttr<NoUniqueAddressAttr>())) + (WasArray || (!AsIfNoUniqueAddr && !FD->hasAttr<NoUniqueAddressAttr>()))) return false; - return isEmptyRecord(Context, FT, AllowArrays); + return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr); } -bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays) { +bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays, + bool AsIfNoUniqueAddr) { const RecordType *RT = T->getAs<RecordType>(); if (!RT) return false; @@ -297,11 +298,11 @@ bool CodeGen::isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays) { // If this is a C++ record, check the bases first. if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD)) for (const auto &I : CXXRD->bases()) - if (!isEmptyRecord(Context, I.getType(), true)) + if (!isEmptyRecord(Context, I.getType(), true, AsIfNoUniqueAddr)) return false; for (const auto *I : RD->fields()) - if (!isEmptyField(Context, I, AllowArrays)) + if (!isEmptyField(Context, I, AllowArrays, AsIfNoUniqueAddr)) return false; return true; } diff --git a/clang/lib/CodeGen/ABIInfoImpl.h b/clang/lib/CodeGen/ABIInfoImpl.h index 5f0cc289af68b3..afde08ba100cf0 100644 --- a/clang/lib/CodeGen/ABIInfoImpl.h +++ b/clang/lib/CodeGen/ABIInfoImpl.h @@ -122,13 +122,19 @@ Address emitMergePHI(CodeGenFunction &CGF, Address Addr1, llvm::BasicBlock *Block2, const llvm::Twine &Name = ""); /// isEmptyField - Return true iff a the field is "empty", that is it -/// is an unnamed bit-field or an (array of) empty record(s). -bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays); +/// is an unnamed bit-field or an (array of) empty record(s). If +/// AsIfNoUniqueAddr is true, then C++ record fields are considered empty if +/// the [[no_unique_address]] attribute would have made them empty. +bool isEmptyField(ASTContext &Context, const FieldDecl *FD, bool AllowArrays, + bool AsIfNoUniqueAddr = false); /// isEmptyRecord - Return true iff a structure contains only empty /// fields. Note that a structure with a flexible array member is not -/// considered empty. -bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays); +/// considered empty. If AsIfNoUniqueAddr is true, then C++ record fields are +/// considered empty if the [[no_unique_address]] attribute would have made +/// them empty. +bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays, + bool AsIfNoUniqueAddr = false); /// isSingleElementStruct - Determine if a structure is a "single /// element struct", i.e. it has exactly one non-empty field or diff --git a/clang/lib/CodeGen/Targets/RISCV.cpp b/clang/lib/CodeGen/Targets/RISCV.cpp index b6d8ae462675c4..4420ff0befaf30 100644 --- a/clang/lib/CodeGen/Targets/RISCV.cpp +++ b/clang/lib/CodeGen/Targets/RISCV.cpp @@ -152,6 +152,13 @@ bool RISCVABIInfo::detectFPCCEligibleStructHelper(QualType Ty, CharUnits CurOff, if (const ConstantArrayType *ATy = getContext().getAsConstantArrayType(Ty)) { uint64_t ArraySize = ATy->getSize().getZExtValue(); QualType EltTy = ATy->getElementType(); + // Non-zero-length arrays of empty records make the struct ineligible for + // the FP calling convention in C++. + if (const auto *RTy = EltTy->getAs<RecordType>()) { + if (ArraySize != 0 && isa<CXXRecordDecl>(RTy->getDecl()) && + isEmptyRecord(getContext(), EltTy, true, true)) + return false; + } CharUnits EltSize = getContext().getTypeSizeInChars(EltTy); for (uint64_t i = 0; i < ArraySize; ++i) { bool Ret = detectFPCCEligibleStructHelper(EltTy, CurOff, Field1Ty, @@ -168,7 +175,7 @@ bool RISCVABIInfo::detectFPCCEligibleStructHelper(QualType Ty, CharUnits CurOff, // copy constructor are not eligible for the FP calling convention. if (getRecordArgABI(Ty, CGT.getCXXABI())) return false; - if (isEmptyRecord(getContext(), Ty, true)) + if (isEmptyRecord(getContext(), Ty, true, true)) return true; const RecordDecl *RD = RTy->getDecl(); // Unions aren't eligible unless they're empty (which is caught above). diff --git a/clang/test/CodeGen/RISCV/abi-empty-structs.c b/clang/test/CodeGen/RISCV/abi-empty-structs.c index 94cc29c95c293d..2a59fbae81ebcc 100644 --- a/clang/test/CodeGen/RISCV/abi-empty-structs.c +++ b/clang/test/CodeGen/RISCV/abi-empty-structs.c @@ -19,8 +19,9 @@ #include <stdint.h> // Fields containing empty structs or unions are ignored when flattening -// structs for the hard FP ABIs, even in C++. -// FIXME: This isn't currently respected. +// structs for the hard FP ABIs, even in C++. The rules for arrays of empty +// structs or unions are subtle and documented in +// <https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc#hardware-floating-point-calling-convention>. struct empty { struct { struct { } e; }; }; struct s1 { struct empty e; float f; }; @@ -29,13 +30,9 @@ struct s1 { struct empty e; float f; }; // CHECK-C-SAME: (float [[TMP0:%.*]]) #[[ATTR0:[0-9]+]] { // CHECK-C: entry: // -// CHECK32-CXX-LABEL: define dso_local [2 x i32] @_Z7test_s12s1 -// CHECK32-CXX-SAME: ([2 x i32] [[A_COERCE:%.*]]) #[[ATTR0:[0-9]+]] { -// CHECK32-CXX: entry: -// -// CHECK64-CXX-LABEL: define dso_local i64 @_Z7test_s12s1 -// CHECK64-CXX-SAME: (i64 [[A_COERCE:%.*]]) #[[ATTR0:[0-9]+]] { -// CHECK64-CXX: entry: +// CHECK-CXX-LABEL: define dso_local float @_Z7test_s12s1 +// CHECK-CXX-SAME: (float [[TMP0:%.*]]) #[[ATTR0:[0-9]+]] { +// CHECK-CXX: entry: // struct s1 test_s1(struct s1 a) { return a; @@ -47,13 +44,9 @@ struct s2 { struct empty e; int32_t i; float f; }; // CHECK-C-SAME: (i32 [[TMP0:%.*]], float [[TMP1:%.*]]) #[[ATTR0]] { // CHECK-C: entry: // -// CHECK32-CXX-LABEL: define dso_local void @_Z7test_s22s2 -// CHECK32-CXX-SAME: (ptr noalias sret([[STRUCT_S2:%.*]]) align 4 [[AGG_RESULT:%.*]], ptr noundef [[A:%.*]]) #[[ATTR0]] { -// CHECK32-CXX: entry: -// -// CHECK64-CXX-LABEL: define dso_local [2 x i64] @_Z7test_s22s2 -// CHECK64-CXX-SAME: ([2 x i64] [[A_COERCE:%.*]]) #[[ATTR0]] { -// CHECK64-CXX: entry: +// CHECK-CXX-LABEL: define dso_local { i32, float } @_Z7test_s22s2 +// CHECK-CXX-SAME: (i32 [[TMP0:%.*]], float [[TMP1:%.*]]) #[[ATTR0]] { +// CHECK-CXX: entry: // struct s2 test_s2(struct s2 a) { return a; @@ -65,13 +58,9 @@ struct s3 { struct empty e; float f; float g; }; // CHECK-C-SAME: (float [[TMP0:%.*]], float [[TMP1:%.*]]) #[[ATTR0]] { // CHECK-C: entry: // -// CHECK32-CXX-LABEL: define dso_local void @_Z7test_s32s3 -// CHECK32-CXX-SAME: (ptr noalias sret([[STRUCT_S3:%.*]]) align 4 [[AGG_RESULT:%.*]], ptr noundef [[A:%.*]]) #[[ATTR0]] { -// CHECK32-CXX: entry: -// -// CHECK64-CXX-LABEL: define dso_local [2 x i64] @_Z7test_s32s3 -// CHECK64-CXX-SAME: ([2 x i64] [[A_COERCE:%.*]]) #[[ATTR0]] { -// CHECK64-CXX: entry: +// CHECK-CXX-LABEL: define dso_local { float, float } @_Z7test_s32s3 +// CHECK-CXX-SAME: (float [[TMP0:%.*]], float [[TMP1:%.*]]) #[[ATTR0]] { +// CHECK-CXX: entry: // struct s3 test_s3(struct s3 a) { return a; @@ -83,13 +72,9 @@ struct s4 { struct empty e; float __complex__ c; }; // CHECK-C-SAME: (float [[TMP0:%.*]], float [[TMP1:%.*]]) #[[ATTR0]] { // CHECK-C: entry: // -// CHECK32-CXX-LABEL: define dso_local void @_Z7test_s42s4 -// CHECK32-CXX-SAME: (ptr noalias sret([[STRUCT_S4:%.*]]) align 4 [[AGG_RESULT:%.*]], ptr noundef [[A:%.*]]) #[[ATTR0]] { -// CHECK32-CXX: entry: -// -// CHECK64-CXX-LABEL: define dso_local [2 x i64] @_Z7test_s42s4 -// CHECK64-CXX-SAME: ([2 x i64] [[A_COERCE:%.*]]) #[[ATTR0]] { -// CHECK64-CXX: entry: +// CHECK-CXX-LABEL: define dso_local { float, float } @_Z7test_s42s4 +// CHECK-CXX-SAME: (float [[TMP0:%.*]], float [[TMP1:%.*]]) #[[ATTR0]] { +// CHECK-CXX: entry: // struct s4 test_s4(struct s4 a) { return a; @@ -142,7 +127,7 @@ struct s7 { struct empty e[0]; float f; }; // CHECK-C: entry: // // CHECK-CXX-LABEL: define dso_local float @_Z7test_s72s7 -// CHECK-CXX-SAME: (float [[TMP0:%.*]]) #[[ATTR0:[0-9]+]] { +// CHECK-CXX-SAME: (float [[TMP0:%.*]]) #[[ATTR0]] { // CHECK-CXX: entry: // struct s7 test_s7(struct s7 a) { @@ -156,13 +141,9 @@ struct s8 { struct empty_arr0 e; float f; }; // CHECK-C-SAME: (float [[TMP0:%.*]]) #[[ATTR0]] { // CHECK-C: entry: // -// CHECK32-CXX-LABEL: define dso_local i32 @_Z7test_s82s8 -// CHECK32-CXX-SAME: (i32 [[A_COERCE:%.*]]) #[[ATTR0]] { -// CHECK32-CXX: entry: -// -// CHECK64-CXX-LABEL: define dso_local i64 @_Z7test_s82s8 -// CHECK64-CXX-SAME: (i64 [[A_COERCE:%.*]]) #[[ATTR0]] { -// CHECK64-CXX: entry: +// CHECK-CXX-LABEL: define dso_local float @_Z7test_s82s8 +// CHECK-CXX-SAME: (float [[TMP0:%.*]]) #[[ATTR0]] { +// CHECK-CXX: entry: // struct s8 test_s8(struct s8 a) { return a; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
