[clang][RISCV] Fix incorrect ABI lowering for inherited structs under hard-float ABIs

[asb]

Summary

The hard float ABIs have a rule that if a flattened struct contains either a single fp value, or an int+fp, or fp+fp then it may be passed in a pair of registers (if sufficient GPRs+FPRs are available). detectFPCCEligibleStruct and the helper it calls, detectFPCCEligibleStructHelper examine the type of the argument/return value to determine if it complies with the requirements for this ABI rule.

As reported in bug #57084, this logic produces incorrect results for C++ structs that inherit from other structs. This is because only the fields of the struct were examined, but enumerating RD->fields misses any fields in inherited C++ structs. This patch corrects that issue by adding appropriate logic to enumerate any included base structs.

[asb]

Add test (I've pre-committed the current behaviour).

Now that update_cc_test_checks.py supports both --function-signature and --filter '^define ' I'd hoped I could massively improve the maintainability of the tests by using it. Unfortunately it seems that merging identical output under common CHECK lines isn't working as it does for update_llc_test_checks.py and the generated CHECK lines don't match against the function return type. It would definitely be good to address these issues and move all the RISC-V ABI tests to using update_cc_test_checks.py, but obviously I leave that for future work in favour of landing a fix quickly.

The previous lowering was broken (as opposed to being working but not in compliance with the official ABI). As such, I don't think there's any argument for providing a flag for the old behaviour (as sometimes happens with other targets after fixing ABI issues).

[jrtc27]

clang/lib/CodeGen/TargetInfo.cpp

10986

With multiple inheritance this offset won't make sense, and it gets particularly fun with diamond virtual inheritance. This is a minefield.

And what about non-standard layout classes in general, like the simple ones that just have a vtable, not just ones that have virtual inheritance somwhere?

[craig.topper]

Remove the WIP note from the end of the description?

[jrtc27]

clang/lib/CodeGen/TargetInfo.cpp

10986

(you at least need uint64_t BaseOffInBits = Layout.getBaseClassOffset(&B) and to add getContext().toCharUnitsFromBits(BaseOffInBits) like the field case, then there's the question of non-standard layout classes)

[craig.topper]

clang/lib/CodeGen/TargetInfo.cpp

10986

If it has a vtable wouldn't it have failed the getRecordArgABI check earlier? I think the copy constructor would not be trivial in that case.

[jrtc27]

clang/lib/CodeGen/TargetInfo.cpp

10986

Quite possibly, I don't know all the details, just enough to be wary of ignoring it entirely

[jrtc27]

clang/test/CodeGen/RISCV/riscv-abi.cpp

1

Not sure why you need -x c++ when it's a .cpp, that should be the default.

Also C++ tests belong in CodeGenCXX surely? Though I do see a lot of .cpp files in CodeGen...

[asb]

Add additional tests for diamond and virtual inheritance.

[kito-cheng]

LGTM, result has compared with GCC for rv32gc/ilp32d and rv64gc/lp64d.

[craig.topper]

clang/test/CodeGen/RISCV/riscv-abi.cpp

98

diamoned -> diamond

clang/lib/CodeGen/TargetInfo.cpp

if (const auto *RTy = Ty->getAs<RecordType>()) {
     if (getRecordArgABI(Ty, CGT.getCXXABI()))
       return false;
     if (isEmptyRecord(getContext(), Ty, 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)) {
+      for (const CXXBaseSpecifier &B : CXXRD->bases()) {
+        const auto *BDecl =
+            cast<CXXRecordDecl>(B.getType()->castAs<RecordType>()->getDecl());
+        CharUnits BaseOff = Layout.getBaseClassOffset(BDecl);
+        bool Ret = detectFPCCEligibleStructHelper(B.getType(), CurOff + BaseOff,
+                                                  Field1Ty, Field1Off, Field2Ty,
+                                                  Field2Off);
+        if (!Ret)
+          return false;
+      }
+    }
     int ZeroWidthBitFieldCount = 0;
     for (const FieldDecl *FD : RD->fields()) {
-      const ASTRecordLayout &Layout = getContext().getASTRecordLayout(RD);
       uint64_t FieldOffInBits = Layout.getFieldOffset(FD->getFieldIndex());
       QualType QTy = FD->getType();
       if (FD->isBitField()) {
         unsigned BitWidth = FD->getBitWidthValue(getContext());
         // Allow a bitfield with a type greater than XLen as long as the
         // bitwidth is XLen or less.
         if (getContext().getTypeSize(QTy) > XLen && BitWidth <= XLen)
           QTy = getContext().getIntTypeForBitwidth(XLen, false);

clang/test/CodeGen/RISCV/riscv-abi.cpp

-// RUN: %clang_cc1 -triple riscv32 -emit-llvm -x c++ %s -o - \
+// RUN: %clang_cc1 -triple riscv32 -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefixes=ILP32,ILP32-ILP32F,ILP32-ILP32F-ILP32D %s
-// RUN: %clang_cc1 -triple riscv32 -target-feature +f -target-abi ilp32f -emit-llvm -x c++ %s -o - \
+// RUN: %clang_cc1 -triple riscv32 -target-feature +f -target-abi ilp32f -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefixes=ILP32F,ILP32-ILP32F,ILP32F-ILP32D,ILP32-ILP32F-ILP32D %s
-// RUN: %clang_cc1 -triple riscv32 -target-feature +f -target-feature +d -target-abi ilp32d -emit-llvm %s -x c++ -o - \
+// RUN: %clang_cc1 -triple riscv32 -target-feature +f -target-feature +d -target-abi ilp32d -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefixes=ILP32D,ILP32F-ILP32D,ILP32-ILP32F-ILP32D %s
 
-// RUN: %clang_cc1 -triple riscv64 -emit-llvm -x c++ %s -o - \
+// RUN: %clang_cc1 -triple riscv64 -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefixes=LP64,LP64-LP64F,LP64-LP64F-LP64D %s
-// RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-abi lp64f -emit-llvm -x c++ %s -o - \
+// RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-abi lp64f -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefixes=LP64F,LP64-LP64F,LP64F-LP64D,LP64-LP64F-LP64D %s
-// RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-feature +d -target-abi lp64d -emit-llvm %s -x c++ -o - \
+// RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-feature +d -target-abi lp64d -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefixes=LP64D,LP64F-LP64D,LP64-LP64F-LP64D %s
 
 #include <stdint.h>
 
 // Ensure that fields inherited from a parent struct are treated in the same
 // way as fields directly in the child for the purposes of RISC-V ABI rules.
 
 struct parent1_int32_s {

 struct parent2_int32_s {
   int32_t i1;
 };
 
 struct child2_float_s : parent2_int32_s {
   float f1;
 };
 
-// TODO: Fix incorrect lowering for hard-float ABIs.
-
 // ILP32: define{{.*}} [2 x i32] @_Z30int32_float_struct_inheritance14child2_float_s([2 x i32] %a.coerce)
-// ILP32F-ILP32D: define{{.*}} float @_Z30int32_float_struct_inheritance14child2_float_s(float %0)
+// ILP32F-ILP32D: define{{.*}} { i32, float } @_Z30int32_float_struct_inheritance14child2_float_s(i32 %0, float %1)
 // LP64: define{{.*}} i64 @_Z30int32_float_struct_inheritance14child2_float_s(i64 %a.coerce)
-// LP64F-LP64D: define{{.*}} float @_Z30int32_float_struct_inheritance14child2_float_s(float %0)
+// LP64F-LP64D: define{{.*}} { i32, float } @_Z30int32_float_struct_inheritance14child2_float_s(i32 %0, float %1)
 struct child2_float_s int32_float_struct_inheritance(struct child2_float_s a) {
   return a;
 }
 
 struct parent3_float_s {
   float f1;
 };
 
 struct child3_int64_s : parent3_float_s {
   int64_t i1;
 };
 
-// TODO: Fix incorrect lowering for lp64f/lp64d ABIs.
-
 // ILP32-ILP32F-ILP32D-LABEL: define{{.*}} void @_Z30float_int64_struct_inheritance14child3_int64_s(ptr noalias sret(%struct.child3_int64_s)
-// LP64-LP64F-LP64D-LABEL: define{{.*}} [2 x i64] @_Z30float_int64_struct_inheritance14child3_int64_s([2 x i64] %a.coerce)
+// LP64-LABEL: define{{.*}} [2 x i64] @_Z30float_int64_struct_inheritance14child3_int64_s([2 x i64] %a.coerce)
+// LP64F-LP64D-LABEL: define{{.*}} { float, i64 } @_Z30float_int64_struct_inheritance14child3_int64_s(float %0, i64 %1)
 struct child3_int64_s float_int64_struct_inheritance(struct child3_int64_s a) {
   return a;
 }
 
 struct parent4_double_s {
   double d1;
 };
 
 struct child4_double_s : parent4_double_s {
   double d1;
 };
 
-// TODO: Fix incorrect lowering for ilp32d/lp64d ABIs.
-
 // ILP32-ILP32F-LABEL: define{{.*}} void @_Z32double_double_struct_inheritance15child4_double_s(ptr noalias sret(%struct.child4_double_s)
-// ILP32D-LABEL: define{{.*}} double @_Z32double_double_struct_inheritance15child4_double_s(double %0)
+// ILP32D-LABEL: define{{.*}} { double, double } @_Z32double_double_struct_inheritance15child4_double_s(double %0, double %1)
 // LP64-LP64F-LABEL: define{{.*}} [2 x i64] @_Z32double_double_struct_inheritance15child4_double_s([2 x i64] %a.coerce)
-// LP64D-LABEL: define{{.*}} double @_Z32double_double_struct_inheritance15child4_double_s(double %0)
+// LP64D-LABEL: define{{.*}} { double, double } @_Z32double_double_struct_inheritance15child4_double_s(double %0, double %1)
 struct child4_double_s double_double_struct_inheritance(struct child4_double_s a) {
   return a;
 }
 
+// When virtual inheritance is used, the resulting struct isn't eligible for
+// passing in registers.
+
+struct parent5_virtual_s {
+  int32_t i1;
+};
+
+struct child5_virtual_s : virtual parent5_virtual_s {
+  float f1;
+};
+
+// ILP32-ILP32F-ILP32D-LABEL: define{{.*}} void @_ZN16child5_virtual_sC1EOS_(ptr noundef nonnull align 4 dereferenceable(8) %this, ptr noundef nonnull align 4 dereferenceable(8) %0)
+// LP64-LP64F-LP64D-LABEL: define{{.*}} void @_ZN16child5_virtual_sC1EOS_(ptr noundef nonnull align 8 dereferenceable(12) %this, ptr noundef nonnull align 8 dereferenceable(12) %0)
+struct child5_virtual_s int32_float_virtual_struct_inheritance(struct child5_virtual_s a) {
+  return a;
+}
+
+// Check for correct lowering in the presence of diamond inheritance.
+
+struct parent6_float_s {
+  float f1;
+};
+
+struct child6a_s : parent6_float_s {
+};
+
+struct child6b_s : parent6_float_s {
+};
+
+struct grandchild_6_s : child6a_s, child6b_s {
+};
+
+// ILP32: define{{.*}} [2 x i32] @_Z38float_float_diamond_struct_inheritance14grandchild_6_s([2 x i32] %a.coerce)
+// ILP32F-ILP64D: define{{.*}} { float, float } @_Z38float_float_diamond_struct_inheritance14grandchild_6_s(float %0, float %1)
+// LP64: define{{.*}} i64 @_Z38float_float_diamond_struct_inheritance14grandchild_6_s(i64 %a.coerce)
+// LP64F-LP64D: define{{.*}} { float, float } @_Z38float_float_diamond_struct_inheritance14grandchild_6_s(float %0, float %1)
+struct grandchild_6_s float_float_diamond_struct_inheritance(struct grandchild_6_s a) {
+  return a;
+}
+
 // NOTE: These prefixes are unused. Do not add tests below this line:
 // ILP32F: {{.*}}
 // LP64F: {{.*}}