asb created this revision. asb added reviewers: kito-cheng, jrtc27, reames, craig.topper, uweigand, luke. Herald added subscribers: wingo, pmatos, VincentWu, vkmr, frasercrmck, evandro, luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, shiva0217, niosHD, sabuasal, simoncook, johnrusso, rbar, arichardson. Herald added a project: All. asb requested review of this revision. Herald added subscribers: cfe-commits, pcwang-thead, eopXD, MaskRay. Herald added a project: clang.
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. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D142327 Files: clang/lib/CodeGen/TargetInfo.cpp clang/test/CodeGen/RISCV/abi-empty-structs.c
Index: clang/test/CodeGen/RISCV/abi-empty-structs.c =================================================================== --- clang/test/CodeGen/RISCV/abi-empty-structs.c +++ clang/test/CodeGen/RISCV/abi-empty-structs.c @@ -29,13 +29,9 @@ // 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 +43,9 @@ // 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 +57,9 @@ // 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 +71,9 @@ // 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; @@ -136,6 +120,5 @@ } //// NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line: -// CHECK-CXX: {{.*}} // CHECK32-C: {{.*}} // CHECK64-C: {{.*}} Index: clang/lib/CodeGen/TargetInfo.cpp =================================================================== --- clang/lib/CodeGen/TargetInfo.cpp +++ clang/lib/CodeGen/TargetInfo.cpp @@ -548,12 +548,13 @@ return Ctx.getOrInsertSyncScopeID(""); /* default sync scope */ } -static bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays); +static bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays, + bool AsIfNoUniqueAddr = false); /// isEmptyField - Return true iff a the field is "empty", that is it /// is an unnamed bit-field or an (array of) empty record(s). static bool isEmptyField(ASTContext &Context, const FieldDecl *FD, - bool AllowArrays) { + bool AllowArrays, bool AsIfNoUniqueAddr = false) { if (FD->isUnnamedBitfield()) return true; @@ -587,16 +588,19 @@ // 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 || (!FD->hasAttr<NoUniqueAddressAttr>() && !AsIfNoUniqueAddr))) return false; - return isEmptyRecord(Context, FT, AllowArrays); + return isEmptyRecord(Context, FT, AllowArrays, AsIfNoUniqueAddr); } /// isEmptyRecord - Return true iff a structure contains only empty /// fields. Note that a structure with a flexible array member is not -/// considered empty. -static 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. +static bool isEmptyRecord(ASTContext &Context, QualType T, bool AllowArrays, + bool AsIfNoUniqueAddr) { const RecordType *RT = T->getAs<RecordType>(); if (!RT) return false; @@ -607,11 +611,11 @@ // 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; } @@ -11080,6 +11084,13 @@ if (const ConstantArrayType *ATy = getContext().getAsConstantArrayType(Ty)) { uint64_t ArraySize = ATy->getSize().getZExtValue(); QualType EltTy = ATy->getElementType(); + // Arrays of empty records make the struct ineligible for the FP calling + // convention in C++. + if (const auto *RTy = EltTy->getAs<RecordType>()) { + if (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, @@ -11096,7 +11107,7 @@ // 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).
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits