[RISCV] Handle empty structs/unions passing in C++ #97315

svs-quic · 2024-07-01T16:31:58Z

According to RISC-V integer calling convention empty structs or union arguments or return values are ignored by C compilers which support them as a non-standard extension. This is not the case for C++, which requires them to be sized types.

Fixes #97285

topperc · 2024-07-01T16:47:21Z

Why is this in Draft?

svs-quic · 2024-07-01T16:49:08Z

Just waiting for the pre-checkins to complete. I can push it for review if I don't really need to wait for it.

Edit: Pushing the patch for review since its late here and I'll need to log off. Also wanted to mention that this change is similar to what LoongArch did in #70320.

llvmbot · 2024-07-01T17:07:13Z

@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-clang-codegen

Author: Sudharsan Veeravalli (svs-quic)

Changes

According to RISC-V integer calling convention empty structs or union arguments or return values are ignored by C compilers which support them as a non-standard extension. This is not the case for C++, which requires them to be sized types.

Fixes #97285

Full diff: https://github.com/llvm/llvm-project/pull/97315.diff

2 Files Affected:

(modified) clang/lib/CodeGen/Targets/RISCV.cpp (+5-4)
(modified) clang/test/CodeGen/RISCV/abi-empty-structs.c (+34)

diff --git a/clang/lib/CodeGen/Targets/RISCV.cpp b/clang/lib/CodeGen/Targets/RISCV.cpp
index a4c5ec315b8df..f2add9351c03c 100644
--- a/clang/lib/CodeGen/Targets/RISCV.cpp
+++ b/clang/lib/CodeGen/Targets/RISCV.cpp
@@ -361,12 +361,13 @@ ABIArgInfo RISCVABIInfo::classifyArgumentType(QualType Ty, bool IsFixed,
                                            CGCXXABI::RAA_DirectInMemory);
   }
 
-  // Ignore empty structs/unions.
-  if (isEmptyRecord(getContext(), Ty, true))
-    return ABIArgInfo::getIgnore();
-
   uint64_t Size = getContext().getTypeSize(Ty);
 
+  // Ignore empty structs/unions whose size is zero. According to the calling
+  // convention empty structs/unions are required to be sized types in C++.
+  if (isEmptyRecord(getContext(), Ty, true) && Size == 0)
+    return ABIArgInfo::getIgnore();
+
   // Pass floating point values via FPRs if possible.
   if (IsFixed && Ty->isFloatingType() && !Ty->isComplexType() &&
       FLen >= Size && ArgFPRsLeft) {
diff --git a/clang/test/CodeGen/RISCV/abi-empty-structs.c b/clang/test/CodeGen/RISCV/abi-empty-structs.c
index c48a2891627d4..877ed4962d4e8 100644
--- a/clang/test/CodeGen/RISCV/abi-empty-structs.c
+++ b/clang/test/CodeGen/RISCV/abi-empty-structs.c
@@ -167,6 +167,40 @@ struct s9 {
 //
 void test_s9(struct s9 a) {}
 
+struct s10 { };
+// CHECK-C-LABEL: define dso_local void @test_s10
+// CHECK-C-SAME: () #[[ATTR0]] {
+// CHECK-C:  entry:
+//
+// CHECK32-CXX-LABEL: define dso_local i32 @_Z8test_s103s10
+// CHECK32-CXX-SAME: (i32 [[A_COERCE:%.*]]) #[[ATTR0]] {
+// CHECK32-CXX:  entry:
+//
+// CHECK64-CXX-LABEL: define dso_local i64 @_Z8test_s103s10
+// CHECK64-CXX-SAME: (i64 [[A_COERCE:%.*]]) #[[ATTR0]] {
+// CHECK64-CXX:  entry:
+//
+struct s10 test_s10(struct s10 a) {
+  return a;
+}
+
+struct s11 { int : 0; };
+// CHECK-C-LABEL: define dso_local void @test_s11
+// CHECK-C-SAME: () #[[ATTR0]] {
+// CHECK-C:  entry:
+//
+// CHECK32-CXX-LABEL: define dso_local i32 @_Z8test_s113s11
+// CHECK32-CXX-SAME: (i32 [[A_COERCE:%.*]]) #[[ATTR0]] {
+// CHECK32-CXX:  entry:
+//
+// CHECK64-CXX-LABEL: define dso_local i64 @_Z8test_s113s11
+// CHECK64-CXX-SAME: (i64 [[A_COERCE:%.*]]) #[[ATTR0]] {
+// CHECK64-CXX:  entry:
+//
+struct s11 test_s11(struct s11 a) {
+  return a;
+}
+
 //// NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
 // CHECK32-C: {{.*}}
 // CHECK64-C: {{.*}}

llvmbot · 2024-07-01T17:07:13Z

@llvm/pr-subscribers-clang

Author: Sudharsan Veeravalli (svs-quic)

Changes

According to RISC-V integer calling convention empty structs or union arguments or return values are ignored by C compilers which support them as a non-standard extension. This is not the case for C++, which requires them to be sized types.

Fixes #97285

Full diff: https://github.com/llvm/llvm-project/pull/97315.diff

2 Files Affected:

(modified) clang/lib/CodeGen/Targets/RISCV.cpp (+5-4)
(modified) clang/test/CodeGen/RISCV/abi-empty-structs.c (+34)

diff --git a/clang/lib/CodeGen/Targets/RISCV.cpp b/clang/lib/CodeGen/Targets/RISCV.cpp
index a4c5ec315b8df..f2add9351c03c 100644
--- a/clang/lib/CodeGen/Targets/RISCV.cpp
+++ b/clang/lib/CodeGen/Targets/RISCV.cpp
@@ -361,12 +361,13 @@ ABIArgInfo RISCVABIInfo::classifyArgumentType(QualType Ty, bool IsFixed,
                                            CGCXXABI::RAA_DirectInMemory);
   }
 
-  // Ignore empty structs/unions.
-  if (isEmptyRecord(getContext(), Ty, true))
-    return ABIArgInfo::getIgnore();
-
   uint64_t Size = getContext().getTypeSize(Ty);
 
+  // Ignore empty structs/unions whose size is zero. According to the calling
+  // convention empty structs/unions are required to be sized types in C++.
+  if (isEmptyRecord(getContext(), Ty, true) && Size == 0)
+    return ABIArgInfo::getIgnore();
+
   // Pass floating point values via FPRs if possible.
   if (IsFixed && Ty->isFloatingType() && !Ty->isComplexType() &&
       FLen >= Size && ArgFPRsLeft) {
diff --git a/clang/test/CodeGen/RISCV/abi-empty-structs.c b/clang/test/CodeGen/RISCV/abi-empty-structs.c
index c48a2891627d4..877ed4962d4e8 100644
--- a/clang/test/CodeGen/RISCV/abi-empty-structs.c
+++ b/clang/test/CodeGen/RISCV/abi-empty-structs.c
@@ -167,6 +167,40 @@ struct s9 {
 //
 void test_s9(struct s9 a) {}
 
+struct s10 { };
+// CHECK-C-LABEL: define dso_local void @test_s10
+// CHECK-C-SAME: () #[[ATTR0]] {
+// CHECK-C:  entry:
+//
+// CHECK32-CXX-LABEL: define dso_local i32 @_Z8test_s103s10
+// CHECK32-CXX-SAME: (i32 [[A_COERCE:%.*]]) #[[ATTR0]] {
+// CHECK32-CXX:  entry:
+//
+// CHECK64-CXX-LABEL: define dso_local i64 @_Z8test_s103s10
+// CHECK64-CXX-SAME: (i64 [[A_COERCE:%.*]]) #[[ATTR0]] {
+// CHECK64-CXX:  entry:
+//
+struct s10 test_s10(struct s10 a) {
+  return a;
+}
+
+struct s11 { int : 0; };
+// CHECK-C-LABEL: define dso_local void @test_s11
+// CHECK-C-SAME: () #[[ATTR0]] {
+// CHECK-C:  entry:
+//
+// CHECK32-CXX-LABEL: define dso_local i32 @_Z8test_s113s11
+// CHECK32-CXX-SAME: (i32 [[A_COERCE:%.*]]) #[[ATTR0]] {
+// CHECK32-CXX:  entry:
+//
+// CHECK64-CXX-LABEL: define dso_local i64 @_Z8test_s113s11
+// CHECK64-CXX-SAME: (i64 [[A_COERCE:%.*]]) #[[ATTR0]] {
+// CHECK64-CXX:  entry:
+//
+struct s11 test_s11(struct s11 a) {
+  return a;
+}
+
 //// NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
 // CHECK32-C: {{.*}}
 // CHECK64-C: {{.*}}

efriedma-quic · 2024-07-01T21:21:12Z

Please add a test for struct X { int x[0]; };.

svs-quic · 2024-07-02T05:24:45Z

Thanks @efriedma-quic. I tried adding a test case for it locally and see that the code produced is different for llvm and gcc: https://godbolt.org/z/vdhGbvj6W

Test case:

struct s12 {int x[0];};
struct s12 test_s12(struct s12 a) {
  return a;
}

For llvm this is an empty record of size zero and so it ignores it where as gcc loads 0 into a0. Do we handle this case similar to what gcc does? Or do I add a fixme for now?

@topperc @asb

efriedma-quic · 2024-07-05T22:20:29Z

The current spec language is:

Empty structs or union arguments or return values are ignored by C compilers which support them as a non-standard extension. This is not the case for C++, which requires them to be sized types.

So empty structs in C are ignored, empty structs in C++ are treated as non-empty.

The "C++ struct containing an empty array" is sort of an edge case, since it's not actually legal C++, but the spec says the distinguishing factor is whether we're in C++ mode. And gcc seems to do that, so let's do the same thing. So instead of checking for Size == 0, check for LangOpts.CPlusPlus.

Also, please add a release note for this.

topperc · 2024-07-05T22:38:29Z

The current spec language is:

Empty structs or union arguments or return values are ignored by C compilers which support them as a non-standard extension. This is not the case for C++, which requires them to be sized types.

So empty structs in C are ignored, empty structs in C++ are treated as non-empty.

The "C++ struct containing an empty array" is sort of an edge case, since it's not actually legal C++, but the spec says the distinguishing factor is whether we're in C++ mode. And gcc seems to do that, so let's do the same thing. So instead of checking for Size == 0, check for LangOpts.CPlusPlus.

Also, please add a release note for this.

gcc does ignore the empty array case for assigning argument registers in C++. Compare the two cases here https://godbolt.org/z/j4WP89rE8

svs-quic · 2024-07-06T04:27:08Z

The current spec language is:

Empty structs or union arguments or return values are ignored by C compilers which support them as a non-standard extension. This is not the case for C++, which requires them to be sized types.

So empty structs in C are ignored, empty structs in C++ are treated as non-empty.

The "C++ struct containing an empty array" is sort of an edge case, since it's not actually legal C++, but the spec says the distinguishing factor is whether we're in C++ mode. And gcc seems to do that, so let's do the same thing. So instead of checking for Size == 0, check for LangOpts.CPlusPlus.

Also, please add a release note for this.

Thanks Eli. I have made these changes.

efriedma-quic · 2024-07-07T18:43:26Z

gcc does ignore the empty array case for assigning argument registers in C++. Compare the two cases here https://godbolt.org/z/j4WP89rE8

Oh, you're right, I didn't actually do any testing myself, I was just assuming @svs-quic did the right test.

So I guess the previous size==0 check was actually correct.

svs-quic · 2024-07-08T03:50:05Z

Thanks @efriedma-quic. I tried adding a test case for it locally and see that the code produced is different for llvm and gcc: https://godbolt.org/z/vdhGbvj6W

Test case:
struct s12 {int x[0];};
struct s12 test_s12(struct s12 a) {
  return a;
}
For llvm this is an empty record of size zero and so it ignores it where as gcc loads 0 into a0. Do we handle this case similar to what gcc does? Or do I add a fixme for now?

@topperc @asb

What about the case I have mentioned in https://godbolt.org/z/vdhGbvj6W ?

Having a check for both Size==0 and LangOpts.CPlusPlus handles this as well.

efriedma-quic · 2024-07-08T07:14:01Z

What about the case I have mentioned in https://godbolt.org/z/vdhGbvj6W ?

That test doesn't really prove anything useful. The zeroing of a0 doesn't have any ABI significance: "struct s12" doesn't have any data in it, so the caller can't use the value in a0 for anything. At most, it's a hint about how gcc's internal logic works.

svs-quic · 2024-07-08T08:25:16Z

Thanks. I have modified the check and added a relevant test for empty arrays.

efriedma-quic

LGTM

topperc

LGTM

svs-quic · 2024-07-09T01:09:51Z

Could someone please merge this change? I do not have permissions to do so.

topperc · 2024-07-09T01:18:01Z

Could someone please merge this change? I do not have permissions to do so.

Done

According to RISC-V integer calling convention empty structs or union arguments or return values are ignored by C compilers which support them as a non-standard extension. This is not the case for C++, which requires them to be sized types. Fixes llvm#97285

[RISCV] Handle empty structs/unions passing in C++

3c744fe

topperc requested review from asb, jrtc27 and kito-cheng July 1, 2024 16:46

svs-quic marked this pull request as ready for review July 1, 2024 17:06

llvmbot added clang Clang issues not falling into any other category backend:RISC-V clang:codegen labels Jul 1, 2024

Add test case

004881e

efriedma-quic self-requested a review July 5, 2024 22:20

Change check and add release notes

d6b523c

Fix check and modify test

8fb4600

efriedma-quic approved these changes Jul 8, 2024

View reviewed changes

topperc approved these changes Jul 8, 2024

View reviewed changes

topperc merged commit d65f423 into llvm:main Jul 9, 2024
8 checks passed

svs-quic deleted the riscv-empty-structs branch July 15, 2024 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RISCV] Handle empty structs/unions passing in C++ #97315

[RISCV] Handle empty structs/unions passing in C++ #97315

svs-quic commented Jul 1, 2024

topperc commented Jul 1, 2024

svs-quic commented Jul 1, 2024 •

edited

Loading

llvmbot commented Jul 1, 2024 •

edited

Loading

llvmbot commented Jul 1, 2024

efriedma-quic commented Jul 1, 2024

svs-quic commented Jul 2, 2024

efriedma-quic commented Jul 5, 2024

topperc commented Jul 5, 2024

svs-quic commented Jul 6, 2024

efriedma-quic commented Jul 7, 2024

svs-quic commented Jul 8, 2024

efriedma-quic commented Jul 8, 2024

svs-quic commented Jul 8, 2024

efriedma-quic left a comment

topperc left a comment

svs-quic commented Jul 9, 2024

topperc commented Jul 9, 2024

[RISCV] Handle empty structs/unions passing in C++ #97315

[RISCV] Handle empty structs/unions passing in C++ #97315

Conversation

svs-quic commented Jul 1, 2024

topperc commented Jul 1, 2024

svs-quic commented Jul 1, 2024 • edited Loading

llvmbot commented Jul 1, 2024 • edited Loading

llvmbot commented Jul 1, 2024

efriedma-quic commented Jul 1, 2024

svs-quic commented Jul 2, 2024

efriedma-quic commented Jul 5, 2024

topperc commented Jul 5, 2024

svs-quic commented Jul 6, 2024

efriedma-quic commented Jul 7, 2024

svs-quic commented Jul 8, 2024

efriedma-quic commented Jul 8, 2024

svs-quic commented Jul 8, 2024

efriedma-quic left a comment

Choose a reason for hiding this comment

topperc left a comment

Choose a reason for hiding this comment

svs-quic commented Jul 9, 2024

topperc commented Jul 9, 2024

svs-quic commented Jul 1, 2024 •

edited

Loading

llvmbot commented Jul 1, 2024 •

edited

Loading