[clang] 17a58b3 - [clang][RISCV] Fix ABI handling of empty structs with hard FP calling conventions in C++
Alex Bradbury via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 24 02:25:11 PDT 2023
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;
More information about the cfe-commits
mailing list