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

[asb]

Summary

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.

[asb]

Removed FIXME missed in initial version.

[luismarques]

LGMT.

clang/lib/CodeGen/TargetInfo.cpp

591 ↗

(On Diff #491237)

Nit: the !AsIfNoUniqueAddr condition could be tested before the !FD->hasAttr<NoUniqueAddressAttr>() condition, and I imagine that would be slightly cheaper for the AsIfNoUniqueAddr = true case?

[asb]

• Address review comments
• Refresh after adding zero-length array tests (and fix a bug this unearthed).
• Add to release notes

[asb]

Thanks Luis for the quick review. As an important part of this is trying to align with gcc/g++ I'd really appreciate a review from @kito-cheng too if you have the time (thanks in advance!).

[asb]

In D142327#4080044, @asb wrote:

Thanks Luis for the quick review. As an important part of this is trying to align with gcc/g++ I'd really appreciate a review from @kito-cheng too if you have the time (thanks in advance!).

Friendly ping to Kito as I spotted you mentioned you're back from vacation!

[asb]

Friendly ping on this (I think mostly directed at @kito-cheng who was hoping to find time to review the linked abi issue).

[kito-cheng]

LGTM, Verified with GCC and two clang w/ and w/o this patch, clang with this patch is matching GCC's behavior.

Also created an PR on psABI to clarify that: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/365

[luismarques]

clang/test/CodeGen/RISCV/abi-empty-structs.c

21–22

Shouldn't you mention the array exception here? (which you now cover in test_s7 and test_s8).

[rogfer01]

clang/lib/CodeGen/Targets/RISCV.cpp

177

I've observed (based on manually added tracing) that we may return true here with Field1Ty == nullptr and Field2Ty == nullptr.

Then RISCVABIInfo::coerceAndExpandFPCCEligibleStruct receives these two types and appends Field1Ty it into UnpaddedCoerceElts and then if Field2Ty == nullptr it calls ABIArgInfo::getCoerceAndExpand (around line 280 of clang/lib/CodeGen/Targets/RISCV.cpp) which then tries to do a dyn_cast on a null value and then an assertion fires (around line 256 of clang/include/clang/CodeGen/CGFunctionInfo.h)

I can reproduce this with the llvm-testsuite. Apologies that I have not managed to create a small reproducer yet.

[asb]

Reopening as I've reverted following Roger's bug report.

clang/lib/CodeGen/Targets/RISCV.cpp

177

Thank you, I've reverted until this can be investigated and fixed.

[SixWeining]

clang/lib/CodeGen/Targets/RISCV.cpp

177

LoongArch has the same issue and I write a reproducer:

$ cat test.cpp
#include <algorithm>
#include <vector>

static bool check(int i) {
  return i == 2;
}

int main()
{
  std::vector<int> v{1, 2, 3, 2, 2};
  //return std::count_if(v.begin(), v.end(), check); // ok
  return std::count_if(v.begin(), v.end(), [](int i) { return i == 2; }); // error
}

Let's investigate how to fix it.

[asb]

I've updated the patch to fix the reported issue (adding an additional check to detectFPCCEligibleStruct). What I haven't been able to do so far is to extract a test case that shows the issue indicated by @SixWeining without relying on libcxx headers. I may well be missing something obvious here.

[rogfer01]

This comment was removed by rogfer01.

[rogfer01]

I didn't have libcxx handy but @SixWeining testcase also fails with libstdcxx so I manually expanded what libstdcxx does and removed the templates.

With the new change this does not fail anymore.

typedef decltype((int *)2 - (int *)1) my_ptrdiff_t;

struct my_lambda {
  bool operator()(int i) { return i == 2; }
};

struct my_iter_pred {
  my_lambda _M_pred;

  explicit my_iter_pred(my_lambda __pred)
      : _M_pred(/* move */ static_cast<my_lambda &&>(__pred)) {}

  bool operator()(int *__it) { return bool(_M_pred(*__it)); }
};

my_ptrdiff_t __my_count_if(int *__first, int *__last, my_iter_pred __pred) {
  my_ptrdiff_t __n = 0;
  for (; __first != __last; ++__first)
    if (__pred(__first))
      ++__n;
  return __n;
}

inline my_iter_pred my_pred_iter(my_lambda __my_lambda) {
  return my_iter_pred(/* move */ static_cast<my_lambda &&>(__my_lambda));
}

inline my_ptrdiff_t my_count_if(int *__first, int *__last,
                                my_lambda __my_lambda) {
  return __my_count_if(__first, __last, my_pred_iter(__my_lambda));
}

int main() {
  int v[] = {1, 2, 3, 2, 2};
  return my_count_if(v, v + sizeof(v) / sizeof(*v), my_lambda());
}

I tried to remove the my_pred_iter / my_iter_pred wrapper but then the previous change would not fail anymore.

Hope this helps.

[asb]

This update includes test coverage for the bug that was fixed in the reverted version of the patch.

Thanks @rogfer01 for the smaller test case. To my surprise, cvise stripped out almost everything and test_s9 in abi-empty-structs.c reliably caused the assert for me.

Re-review appreciated in order to get this landed and ideally cherry-picked for the 17.x.

[rogfer01]

Thanks for the update @asb. LGTM.

clang/docs/ReleaseNotes.rst

 
 LoongArch Support
 ^^^^^^^^^^^^^^^^^
 
 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
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 
 CUDA Support
 ^^^^^^^^^^^^
 
 AIX Support

clang/lib/CodeGen/ABIInfoImpl.h

Address emitVoidPtrVAArg(CodeGenFunction &CGF, Address VAListAddr,
                          TypeInfoChars ValueInfo, CharUnits SlotSizeAndAlign,
                          bool AllowHigherAlign, bool ForceRightAdjust = false);
 
 Address emitMergePHI(CodeGenFunction &CGF, Address Addr1,
                      llvm::BasicBlock *Block1, Address Addr2,
                      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
 /// exactly one field which is itself a single element
 /// struct. Structures with flexible array members are never
 /// considered single element structs.
 ///
 /// \return The field declaration for the single non-empty field, if

clang/lib/CodeGen/ABIInfoImpl.cpp

Address CodeGen::emitMergePHI(CodeGenFunction &CGF, Address Addr1,
   llvm::PHINode *PHI = CGF.Builder.CreatePHI(Addr1.getType(), 2, Name);
   PHI->addIncoming(Addr1.getPointer(), Block1);
   PHI->addIncoming(Addr2.getPointer(), Block2);
   CharUnits Align = std::min(Addr1.getAlignment(), Addr2.getAlignment());
   return Address(PHI, Addr1.getElementType(), Align);
 }
 
 bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
-                           bool AllowArrays) {
+                           bool AllowArrays, bool AsIfNoUniqueAddr) {
   if (FD->isUnnamedBitfield())
     return true;
 
   QualType FT = FD->getType();
 
   // Constant arrays of empty records count as empty, strip them off.
   // Constant arrays of zero length always count as empty.
   bool WasArray = false;
bool CodeGen::isEmptyField(ASTContext &Context, const FieldDecl *FD,
   // current ABI.
   //
   // The exception to the above rule are fields marked with the
   // [[no_unique_address]] attribute (since C++20).  Those do count as empty
   // according to the Itanium ABI.  The exception applies only to records,
   // 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;
   const RecordDecl *RD = RT->getDecl();
   if (RD->hasFlexibleArrayMember())
     return false;
 
   // 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;
 }
 
 const Type *CodeGen::isSingleElementStruct(QualType T, ASTContext &Context) {
   const RecordType *RT = T->getAs<RecordType>();
   if (!RT)
     return nullptr;

clang/lib/CodeGen/Targets/RISCV.cpp

if (auto CTy = Ty->getAs<ComplexType>()) {
     Field2Ty = Field1Ty;
     Field2Off = Field1Off + getContext().getTypeSizeInChars(EltTy);
     return true;
   }
 
   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,
                                                 Field1Off, Field2Ty, Field2Off);
       if (!Ret)
         return false;
       CurOff += EltSize;
     }
     return true;
   }
 
   if (const auto *RTy = Ty->getAs<RecordType>()) {
     // Structures with either a non-trivial destructor or a non-trivial
     // 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).
     if (RD->isUnion())
       return false;
     const ASTRecordLayout &Layout = getContext().getASTRecordLayout(RD);
     // If this is a C++ record, check the bases first.
     if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
bool RISCVABIInfo::detectFPCCEligibleStruct(QualType Ty, llvm::Type *&Field1Ty,
                                             int &NeededArgGPRs,
                                             int &NeededArgFPRs) const {
   Field1Ty = nullptr;
   Field2Ty = nullptr;
   NeededArgGPRs = 0;
   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;
   if (!IsCandidate)
     return false;
   if (Field1Ty && Field1Ty->isFloatingPointTy())
     NeededArgFPRs++;
   else if (Field1Ty)

clang/test/CodeGen/RISCV/abi-empty-structs.c

-// 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 - \
 // RUN:   | FileCheck -check-prefixes=CHECK-C,CHECK32-C %s
 // RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-abi lp64f -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefixes=CHECK-C,CHECK64-C %s
 // RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-feature +d -target-abi lp64d -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefixes=CHECK-C,CHECK64-C %s
 // RUN: %clang_cc1 -x c++ -triple riscv32 -target-feature +f -target-abi ilp32f -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefixes=CHECK-CXX,CHECK32-CXX %s
 // RUN: %clang_cc1 -x c++ -triple riscv32 -target-feature +f -target-feature +d -target-abi ilp32d -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefixes=CHECK-CXX,CHECK32-CXX %s
 // RUN: %clang_cc1 -x c++ -triple riscv64 -target-feature +f -target-abi lp64f -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefixes=CHECK-CXX,CHECK64-CXX %s
 // RUN: %clang_cc1 -x c++ -triple riscv64 -target-feature +f -target-feature +d -target-abi lp64d -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefixes=CHECK-CXX,CHECK64-CXX %s
 
 #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; };
 
 // CHECK-C-LABEL: define dso_local float @test_s1
 // 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;
 }
 
 struct s2 { struct empty e; int32_t i; float f; };
 
 // CHECK-C-LABEL: define dso_local { i32, float } @test_s2
 // 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;
 }
 
 struct s3 { struct empty e; float f; float g; };
 
 // CHECK-C-LABEL: define dso_local { float, float } @test_s3
 // 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;
 }
 
 struct s4 { struct empty e; float __complex__ c; };
 
 // CHECK-C-LABEL: define dso_local { float, float } @test_s4
 // 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;
 }
 
 // An array of empty fields isn't ignored in C++ (this isn't explicit in the
 // psABI, but matches observed g++ behaviour).
 

 
 struct s7 { struct empty e[0]; float f; };
 
 // CHECK-C-LABEL: define dso_local float @test_s7
 // CHECK-C-SAME: (float [[TMP0:%.*]]) #[[ATTR0]] {
 // 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) {
   return a;
 }
 
 struct empty_arr0 { struct { struct { } e[0]; }; };
 struct s8 { struct empty_arr0 e; float f; };
 
 // CHECK-C-LABEL: define dso_local float @test_s8
 // 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: {{.*}}
 // CHECK64-C: {{.*}}