[clang] e3c57fd - [clang][RISCV] Fix bug in ABI handling of empty structs with hard FP calling conventions in C++
Alex Bradbury via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 7 02:45:51 PDT 2023
Author: Alex Bradbury
Date: 2023-08-07T10:45:22+01:00
New Revision: e3c57fdd8439ba82c67347629a3c66f293e1f3d0
URL: https://github.com/llvm/llvm-project/commit/e3c57fdd8439ba82c67347629a3c66f293e1f3d0
DIFF: https://github.com/llvm/llvm-project/commit/e3c57fdd8439ba82c67347629a3c66f293e1f3d0.diff
LOG: [clang][RISCV] Fix bug in 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'). It isn't explicitly noted in the RISC-V
psABI, but arrays of empty records will disqualify a struct for
consideration of using the FP calling convention in g++. This patch
matches that behaviour. The psABI issue
<https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/358> seeks
to clarify this.
This patch was previously committed but reverted after a bug was found.
This recommit adds additional logic to prevent that bug (adding an extra
check for when a candidate from detectFPCCEligibleStructHelper may not
be valid).
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 f03e5231215eb2..4d1ca7afad1f10 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -196,6 +196,9 @@ RISC-V Support
^^^^^^^^^^^^^^
- Unaligned memory accesses can be toggled by ``-m[no-]unaligned-access`` or the
aliases ``-m[no-]strict-align``.
+- 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 5926cee24f3a23..b12c3025f60738 100644
--- a/clang/lib/CodeGen/Targets/RISCV.cpp
+++ b/clang/lib/CodeGen/Targets/RISCV.cpp
@@ -151,6 +151,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,
@@ -167,7 +174,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).
@@ -237,6 +244,8 @@ bool RISCVABIInfo::detectFPCCEligibleStruct(QualType Ty, llvm::Type *&Field1Ty,
NeededArgFPRs = 0;
bool IsCandidate = detectFPCCEligibleStructHelper(
Ty, CharUnits::Zero(), Field1Ty, Field1Off, Field2Ty, Field2Off);
+ if (!Field1Ty)
+ return false;
// Not really a candidate if we have a single int but no float.
if (Field1Ty && !Field2Ty && !Field1Ty->isFloatingPointTy())
return false;
diff --git a/clang/test/CodeGen/RISCV/abi-empty-structs.c b/clang/test/CodeGen/RISCV/abi-empty-structs.c
index 94cc29c95c293d..c48a2891627d42 100644
--- a/clang/test/CodeGen/RISCV/abi-empty-structs.c
+++ b/clang/test/CodeGen/RISCV/abi-empty-structs.c
@@ -1,4 +1,4 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --function-signature --full-function-signature --filter "^define |^entry:"
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --filter "^define |^entry:" --version 2
// RUN: %clang_cc1 -triple riscv32 -target-feature +f -target-abi ilp32f -emit-llvm %s -o - \
// RUN: | FileCheck -check-prefixes=CHECK-C,CHECK32-C %s
// RUN: %clang_cc1 -triple riscv32 -target-feature +f -target-feature +d -target-abi ilp32d -emit-llvm %s -o - \
@@ -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,17 +141,31 @@ 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
+// 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;
+}
+
+struct s9 {
+ struct empty e;
+};
+
+// CHECK-C-LABEL: define dso_local void @test_s9
+// CHECK-C-SAME: () #[[ATTR0]] {
+// CHECK-C: entry:
+//
+// CHECK32-CXX-LABEL: define dso_local void @_Z7test_s92s9
// CHECK32-CXX-SAME: (i32 [[A_COERCE:%.*]]) #[[ATTR0]] {
// CHECK32-CXX: entry:
//
-// CHECK64-CXX-LABEL: define dso_local i64 @_Z7test_s82s8
+// CHECK64-CXX-LABEL: define dso_local void @_Z7test_s92s9
// CHECK64-CXX-SAME: (i64 [[A_COERCE:%.*]]) #[[ATTR0]] {
// CHECK64-CXX: entry:
//
-struct s8 test_s8(struct s8 a) {
- return a;
-}
+void test_s9(struct s9 a) {}
//// NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
// CHECK32-C: {{.*}}
More information about the cfe-commits
mailing list