[clang][RISCV] Fix bug in ABI handling of empty structs with hard FP calling conventions in C++

As reported in <llvm#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
<riscv-non-isa/riscv-elf-psabi-doc#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

Author: asb

clang/docs/ReleaseNotes.rst

@@ -945,6 +945,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.
 - Support the RVV intrinsics v0.12. Please checkout `the RVV C intrinsics
   specification
   <https://github.com/riscv-non-isa/rvv-intrinsic-doc/releases/tag/v0.12.0>`_.

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;
 }

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

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;

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: {{.*}}