[PATCH] LoongArch: Fix the problem of structure parameter passing in C++. This structure has empty structure members and less than three floating point members.

[PATCH] LoongArch: Fix the problem of structure parameter passing in C++. This structure has empty structure members and less than three floating point members.

[Lulu Cheng]

An empty struct type that is not non-trivial for the purposes of calls
will be treated as though it were the following C type:

struct {
  char c;
};

Before this patch was added, a structure parameter containing an empty structure and
less than three floating-point members was passed through one or two floating-point
registers, while nested empty structures are ignored. Which did not conform to the
calling convention.

After adding this patch, empty structs will always be passed.

gcc/ChangeLog:

	* config/loongarch/loongarch.cc (loongarch_flatten_aggregate_field):
	Mark out nested empty structs.
	(loongarch_pass_aggregate_in_fpr_and_gpr_p): If an empty structure exists,
	increment the number of fixed-point registers required.

gcc/testsuite/ChangeLog:

	* g++.target/loongarch/empty1.C: New test.
	* g++.target/loongarch/empty2.C: New test.
	* g++.target/loongarch/empty3.C: New test.
---
 gcc/config/loongarch/loongarch.cc           | 13 +++++++++++++
 gcc/testsuite/g++.target/loongarch/empty1.C | 18 ++++++++++++++++++
 gcc/testsuite/g++.target/loongarch/empty2.C | 20 ++++++++++++++++++++
 gcc/testsuite/g++.target/loongarch/empty3.C | 19 +++++++++++++++++++
 4 files changed, 70 insertions(+)
 create mode 100644 gcc/testsuite/g++.target/loongarch/empty1.C
 create mode 100644 gcc/testsuite/g++.target/loongarch/empty2.C
 create mode 100644 gcc/testsuite/g++.target/loongarch/empty3.C

diff --git a/gcc/config/loongarch/loongarch.cc b/gcc/config/loongarch/loongarch.cc
index 7f4e0e59573..77506410501 100644
--- a/gcc/config/loongarch/loongarch.cc
+++ b/gcc/config/loongarch/loongarch.cc
@@ -321,6 +321,18 @@ loongarch_flatten_aggregate_field (const_tree type,
 	  || !tree_fits_uhwi_p (TYPE_SIZE (type)))
 	return -1;
 
+      if (default_is_empty_record (type))
+	{
+	  if (n < 2)
+	    {
+	      fields[n].type = type;
+	      fields[n].offset = offset;
+	      return n + 1;
+	    }
+	  else
+	    return -1;
+	}
+
       for (tree f = TYPE_FIELDS (type); f; f = DECL_CHAIN (f))
 	if (TREE_CODE (f) == FIELD_DECL)
 	  {
@@ -458,6 +470,7 @@ loongarch_pass_aggregate_in_fpr_and_gpr_p (const_tree type,
     {
       num_float += SCALAR_FLOAT_TYPE_P (fields[i].type);
       num_int += INTEGRAL_TYPE_P (fields[i].type);
+      num_int += (TREE_CODE (fields[i].type) == RECORD_TYPE);
     }
 
   return num_int == 1 && num_float == 1;
diff --git a/gcc/testsuite/g++.target/loongarch/empty1.C b/gcc/testsuite/g++.target/loongarch/empty1.C
new file mode 100644
index 00000000000..059e6774158
--- /dev/null
+++ b/gcc/testsuite/g++.target/loongarch/empty1.C
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -mabi=lp64d" } */
+/* { dg-final { scan-assembler-times "ld.bu\t\\\$r4,\\\$r12,0" 1 } } */
+
+struct E {};
+struct s1
+{
+  struct E {} e1;
+  float f0;
+};
+
+extern void fun (struct s1);
+
+struct s1 s1_1 = {{}, 1.0};
+void test (void)
+{
+  fun (s1_1);
+}
diff --git a/gcc/testsuite/g++.target/loongarch/empty2.C b/gcc/testsuite/g++.target/loongarch/empty2.C
new file mode 100644
index 00000000000..abf01de751a
--- /dev/null
+++ b/gcc/testsuite/g++.target/loongarch/empty2.C
@@ -0,0 +1,20 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -mabi=lp64d" } */
+/* { dg-final { scan-assembler-not "fld.s" } } */
+/* { dg-final { scan-assembler-not "fld.d" } } */
+
+struct E {};
+struct s1
+{
+  struct E {} e1;
+  float f0;
+  double f1;
+};
+
+extern void fun (struct s1);
+
+struct s1 s1_1 = {{}, 1.0, 2.0};
+void test (void)
+{
+  fun (s1_1);
+}
diff --git a/gcc/testsuite/g++.target/loongarch/empty3.C b/gcc/testsuite/g++.target/loongarch/empty3.C
new file mode 100644
index 00000000000..8c40963238b
--- /dev/null
+++ b/gcc/testsuite/g++.target/loongarch/empty3.C
@@ -0,0 +1,19 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -mabi=lp64d" } */
+/* { dg-final { scan-assembler-not "fld.d" } } */
+
+struct E {};
+struct s1
+{
+  struct E {} e1;
+  double f0;
+  double f1;
+};
+
+extern void fun (struct s1);
+
+struct s1 s1_1 = {{}, 1.0, 2.0};
+void test (void)
+{
+  fun (s1_1);
+}
-- 
2.31.1

Re: [PATCH] LoongArch: Fix the problem of structure parameter passing in C++. This structure has empty structure members and less than three floating point members.

[Xi Ruoyao]

On Wed, 2023-05-24 at 14:04 +0800, Lulu Cheng wrote:
> An empty struct type that is not non-trivial for the purposes of calls
> will be treated as though it were the following C type:
> 
> struct {
>   char c;
> };
> 
> Before this patch was added, a structure parameter containing an empty structure and
> less than three floating-point members was passed through one or two floating-point
> registers, while nested empty structures are ignored. Which did not conform to the
> calling convention.

No, it's a deliberate decision I've made in
https://gcc.gnu.org/r12-8294.  And we already agreed "the ABI needs to
be updated" when we applied r12-8294, but I've never improved my English
skill to revise the ABI myself :(.

We are also using the same "de-facto" ABI throwing away the empty struct
for Clang++ (https://reviews.llvm.org/D132285).  So we should update the
spec here, instead of changing every implementation.

The C++ standard treats the empty struct as size 1 for ensuring the
semantics of pointer comparison operations.  When we pass it through the
registers, there is no need to really consider the empty field because
there is no pointers to registers.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] LoongArch: Fix the problem of structure parameter passing in C++. This structure has empty structure members and less than three floating point members.

[Xi Ruoyao]

Wang Lei raised some concerns about Itanium C++ ABI, so let's ask a C++
expert here...

Jonathan: AFAIK the standard and the Itanium ABI treats an empty class
as size 1 in order to guarantee unique address, so for the following:

class Empty {};
class Test { Empty empty; double a, b; };

When we pass "Test" via registers, we may only allocate the registers
for Test::a and Test::b, and complete ignore Test::empty because there
is no addresses of registers.  Is this correct or not?

On Wed, 2023-05-24 at 14:45 +0800, Xi Ruoyao via Gcc-patches wrote:
> On Wed, 2023-05-24 at 14:04 +0800, Lulu Cheng wrote:
> > An empty struct type that is not non-trivial for the purposes of
> > calls
> > will be treated as though it were the following C type:
> > 
> > struct {
> >   char c;
> > };
> > 
> > Before this patch was added, a structure parameter containing an
> > empty structure and
> > less than three floating-point members was passed through one or two
> > floating-point
> > registers, while nested empty structures are ignored. Which did not
> > conform to the
> > calling convention.
> 
> No, it's a deliberate decision I've made in
> https://gcc.gnu.org/r12-8294.  And we already agreed "the ABI needs to
> be updated" when we applied r12-8294, but I've never improved my
> English
> skill to revise the ABI myself :(.
> 
> We are also using the same "de-facto" ABI throwing away the empty
> struct
> for Clang++ (https://reviews.llvm.org/D132285).  So we should update
> the
> spec here, instead of changing every implementation.
> 
> The C++ standard treats the empty struct as size 1 for ensuring the
> semantics of pointer comparison operations.  When we pass it through
> the
> registers, there is no need to really consider the empty field because
> there is no pointers to registers.
> 

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] LoongArch: Fix the problem of structure parameter passing in C++. This structure has empty structure members and less than three floating point members.

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 2400 bytes --]

On Wed, 24 May 2023 at 09:41, Xi Ruoyao <xry111@xry111.site> wrote:

> Wang Lei raised some concerns about Itanium C++ ABI, so let's ask a C++
> expert here...
>
> Jonathan: AFAIK the standard and the Itanium ABI treats an empty class
> as size 1


Only as a complete object, not as a subobject.


> in order to guarantee unique address, so for the following:
>
> class Empty {};
> class Test { Empty empty; double a, b; };
>

There is no need to have a unique address here, so Test::empty and Test::a
have the same address. It's a potentially-overlapping subobject.

For the Itanium ABI, sizeof(Test) == 2 * sizeof(double).



> When we pass "Test" via registers, we may only allocate the registers
> for Test::a and Test::b, and complete ignore Test::empty because there
> is no addresses of registers.  Is this correct or not?
>

I think that's a decision for the loongarch psABI. In principle, there's no
reason a register has to be used to pass Test::empty, since you can't read
from it or write to it.



>
> On Wed, 2023-05-24 at 14:45 +0800, Xi Ruoyao via Gcc-patches wrote:
> > On Wed, 2023-05-24 at 14:04 +0800, Lulu Cheng wrote:
> > > An empty struct type that is not non-trivial for the purposes of
> > > calls
> > > will be treated as though it were the following C type:
> > >
> > > struct {
> > >   char c;
> > > };
> > >
> > > Before this patch was added, a structure parameter containing an
> > > empty structure and
> > > less than three floating-point members was passed through one or two
> > > floating-point
> > > registers, while nested empty structures are ignored. Which did not
> > > conform to the
> > > calling convention.
> >
> > No, it's a deliberate decision I've made in
> > https://gcc.gnu.org/r12-8294.  And we already agreed "the ABI needs to
> > be updated" when we applied r12-8294, but I've never improved my
> > English
> > skill to revise the ABI myself :(.
> >
> > We are also using the same "de-facto" ABI throwing away the empty
> > struct
> > for Clang++ (https://reviews.llvm.org/D132285).  So we should update
> > the
> > spec here, instead of changing every implementation.
> >
> > The C++ standard treats the empty struct as size 1 for ensuring the
> > semantics of pointer comparison operations.  When we pass it through
> > the
> > registers, there is no need to really consider the empty field because
> > there is no pointers to registers.
> >
>
>

Re: [PATCH] LoongArch: Fix the problem of structure parameter passing in C++. This structure has empty structure members and less than three floating point members.

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 3630 bytes --]

On Wed, May 24, 2023 at 5:00 AM Jonathan Wakely via Gcc-patches <
gcc-patches@gcc.gnu.org> wrote:

> On Wed, 24 May 2023 at 09:41, Xi Ruoyao <xry111@xry111.site> wrote:
>
> > Wang Lei raised some concerns about Itanium C++ ABI, so let's ask a C++
> > expert here...
> >
> > Jonathan: AFAIK the standard and the Itanium ABI treats an empty class
> > as size 1
>
> Only as a complete object, not as a subobject.
>

Also as a data member subobject.


> > in order to guarantee unique address, so for the following:
> >
> > class Empty {};
> > class Test { Empty empty; double a, b; };
>
> There is no need to have a unique address here, so Test::empty and Test::a
> have the same address. It's a potentially-overlapping subobject.
>
> For the Itanium ABI, sizeof(Test) == 2 * sizeof(double).
>

That would be true if Test::empty were marked [[no_unique_address]], but
without that attribute, sizeof(Test) is actually 3 * sizeof(double).


> > When we pass "Test" via registers, we may only allocate the registers
> > for Test::a and Test::b, and complete ignore Test::empty because there
> > is no addresses of registers.  Is this correct or not?
>
> I think that's a decision for the loongarch psABI. In principle, there's no
> reason a register has to be used to pass Test::empty, since you can't read
> from it or write to it.
>

Agreed.  The Itanium C++ ABI has nothing to say about how registers are
allocated for parameter passing; this is a matter for the psABI.

And there is no need for a psABI to allocate a register for Test::empty
because it contains no data.

In the x86_64 psABI, Test above is passed in memory because of its size
("the size of the aggregate exceeds two eightbytes...").  But

struct Test2 { Empty empty; double a; };

is passed in a single floating-point register; the Test2::empty subobject
is not passed anywhere, because its eightbyte is classified as NO_CLASS,
because there is no actual data there.

I know nothing about the LoongArch psABI, but going out of your way to
assign a register to an empty class seems like a mistake.

> On Wed, 2023-05-24 at 14:45 +0800, Xi Ruoyao via Gcc-patches wrote:
> > > On Wed, 2023-05-24 at 14:04 +0800, Lulu Cheng wrote:
> > > > An empty struct type that is not non-trivial for the purposes of
> > > > calls
> > > > will be treated as though it were the following C type:
> > > >
> > > > struct {
> > > >   char c;
> > > > };
> > > >
> > > > Before this patch was added, a structure parameter containing an
> > > > empty structure and
> > > > less than three floating-point members was passed through one or two
> > > > floating-point
> > > > registers, while nested empty structures are ignored. Which did not
> > > > conform to the
> > > > calling convention.
> > >
> > > No, it's a deliberate decision I've made in
> > > https://gcc.gnu.org/r12-8294.  And we already agreed "the ABI needs to
> > > be updated" when we applied r12-8294, but I've never improved my
> > > English
> > > skill to revise the ABI myself :(.
> > >
> > > We are also using the same "de-facto" ABI throwing away the empty
> > > struct
> > > for Clang++ (https://reviews.llvm.org/D132285).  So we should update
> > > the
> > > spec here, instead of changing every implementation.
> > >
> > > The C++ standard treats the empty struct as size 1 for ensuring the
> > > semantics of pointer comparison operations.  When we pass it through
> > > the
> > > registers, there is no need to really consider the empty field because
> > > there is no pointers to registers.
> > >
> >
> >
>
>

Re: [PATCH] LoongArch: Fix the problem of structure parameter passing in C++. This structure has empty structure members and less than three floating point members.

[Lulu Cheng]

[-- Attachment #1: Type: text/plain, Size: 4448 bytes --]


在 2023/5/25 上午4:15, Jason Merrill 写道:
> On Wed, May 24, 2023 at 5:00 AM Jonathan Wakely via Gcc-patches 
> <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote:
>
>     On Wed, 24 May 2023 at 09:41, Xi Ruoyao <xry111@xry111.site> wrote:
>
>     > Wang Lei raised some concerns about Itanium C++ ABI, so let's
>     ask a C++
>     > expert here...
>     >
>     > Jonathan: AFAIK the standard and the Itanium ABI treats an empty
>     class
>     > as size 1
>
>     Only as a complete object, not as a subobject.
>
>
> Also as a data member subobject.
>
>     > in order to guarantee unique address, so for the following:
>     >
>     > class Empty {};
>     > class Test { Empty empty; double a, b; };
>
>     There is no need to have a unique address here, so Test::empty and
>     Test::a
>     have the same address. It's a potentially-overlapping subobject.
>
>     For the Itanium ABI, sizeof(Test) == 2 * sizeof(double).
>
>
> That would be true if Test::empty were marked [[no_unique_address]], 
> but without that attribute, sizeof(Test) is actually 3 * sizeof(double).
>
>     > When we pass "Test" via registers, we may only allocate the
>     registers
>     > for Test::a and Test::b, and complete ignore Test::empty because
>     there
>     > is no addresses of registers.  Is this correct or not?
>
>     I think that's a decision for the loongarch psABI. In principle,
>     there's no
>     reason a register has to be used to pass Test::empty, since you
>     can't read
>     from it or write to it.
>
>
> Agreed.  The Itanium C++ ABI has nothing to say about how registers 
> are allocated for parameter passing; this is a matter for the psABI.
>
> And there is no need for a psABI to allocate a register for 
> Test::empty because it contains no data.
>
> In the x86_64 psABI, Test above is passed in memory because of its 
> size ("the size of the aggregate exceeds two eightbytes...").  But
>
> struct Test2 { Empty empty; double a; };
>
> is passed in a single floating-point register; the Test2::empty 
> subobject is not passed anywhere, because its eightbyte is classified 
> as NO_CLASS, because there is no actual data there.



>
> I know nothing about the LoongArch psABI, but going out of your way to 
> assign a register to an empty class seems like a mistake.

MIPS64 and ARM64 also allocate parameter registers for empty structs. 
https://godbolt.org/z/jT4cY3T5o

Our original intention is not to pass this empty structure member, but 
to make the following two structures treat empty structure members

in the same way in the process of passing parameters.

struct st1
{
     struct empty {} e1;
     long a;
     long b;
};

struct st2
{
     struct empty {} e1;
     double f0;
     double f1;
};


>
>     > On Wed, 2023-05-24 at 14:45 +0800, Xi Ruoyao via Gcc-patches wrote:
>     > > On Wed, 2023-05-24 at 14:04 +0800, Lulu Cheng wrote:
>     > > > An empty struct type that is not non-trivial for the purposes of
>     > > > calls
>     > > > will be treated as though it were the following C type:
>     > > >
>     > > > struct {
>     > > >   char c;
>     > > > };
>     > > >
>     > > > Before this patch was added, a structure parameter containing an
>     > > > empty structure and
>     > > > less than three floating-point members was passed through
>     one or two
>     > > > floating-point
>     > > > registers, while nested empty structures are ignored. Which
>     did not
>     > > > conform to the
>     > > > calling convention.
>     > >
>     > > No, it's a deliberate decision I've made in
>     > > https://gcc.gnu.org/r12-8294. And we already agreed "the ABI
>     needs to
>     > > be updated" when we applied r12-8294, but I've never improved my
>     > > English
>     > > skill to revise the ABI myself :(.
>     > >
>     > > We are also using the same "de-facto" ABI throwing away the empty
>     > > struct
>     > > for Clang++ (https://reviews.llvm.org/D132285). So we should
>     update
>     > > the
>     > > spec here, instead of changing every implementation.
>     > >
>     > > The C++ standard treats the empty struct as size 1 for
>     ensuring the
>     > > semantics of pointer comparison operations.  When we pass it
>     through
>     > > the
>     > > registers, there is no need to really consider the empty field
>     because
>     > > there is no pointers to registers.
>     > >
>     >
>     >
>

Re: [PATCH] LoongArch: Fix the problem of structure parameter passing in C++. This structure has empty structure members and less than three floating point members.

[WANG Xuerui]

On 2023/5/25 10:46, Lulu Cheng wrote:
> 
> 在 2023/5/25 上午4:15, Jason Merrill 写道:
>> On Wed, May 24, 2023 at 5:00 AM Jonathan Wakely via Gcc-patches 
>> <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote:
>>
>>     On Wed, 24 May 2023 at 09:41, Xi Ruoyao <xry111@xry111.site> wrote:
>>
>>     > Wang Lei raised some concerns about Itanium C++ ABI, so let's
>>     ask a C++
>>     > expert here...
>>     >
>>     > Jonathan: AFAIK the standard and the Itanium ABI treats an empty
>>     class
>>     > as size 1
>>
>>     Only as a complete object, not as a subobject.
>>
>>
>> Also as a data member subobject.
>>
>>     > in order to guarantee unique address, so for the following:
>>     >
>>     > class Empty {};
>>     > class Test { Empty empty; double a, b; };
>>
>>     There is no need to have a unique address here, so Test::empty and
>>     Test::a
>>     have the same address. It's a potentially-overlapping subobject.
>>
>>     For the Itanium ABI, sizeof(Test) == 2 * sizeof(double).
>>
>>
>> That would be true if Test::empty were marked [[no_unique_address]], 
>> but without that attribute, sizeof(Test) is actually 3 * sizeof(double).
>>
>>     > When we pass "Test" via registers, we may only allocate the
>>     registers
>>     > for Test::a and Test::b, and complete ignore Test::empty because
>>     there
>>     > is no addresses of registers.  Is this correct or not?
>>
>>     I think that's a decision for the loongarch psABI. In principle,
>>     there's no
>>     reason a register has to be used to pass Test::empty, since you
>>     can't read
>>     from it or write to it.
>>
>>
>> Agreed.  The Itanium C++ ABI has nothing to say about how registers 
>> are allocated for parameter passing; this is a matter for the psABI.
>>
>> And there is no need for a psABI to allocate a register for 
>> Test::empty because it contains no data.
>>
>> In the x86_64 psABI, Test above is passed in memory because of its 
>> size ("the size of the aggregate exceeds two eightbytes...").  But
>>
>> struct Test2 { Empty empty; double a; };
>>
>> is passed in a single floating-point register; the Test2::empty 
>> subobject is not passed anywhere, because its eightbyte is classified 
>> as NO_CLASS, because there is no actual data there.
> 
> 
> 
>>
>> I know nothing about the LoongArch psABI, but going out of your way to 
>> assign a register to an empty class seems like a mistake.
> 
> MIPS64 and ARM64 also allocate parameter registers for empty structs. 
> https://godbolt.org/z/jT4cY3T5o
> 
> Our original intention is not to pass this empty structure member, but 
> to make the following two structures treat empty structure members
> 
> in the same way in the process of passing parameters.
> 
> struct st1
> {
>      struct empty {} e1;
>      long a;
>      long b;
> };
> 
> struct st2
> {
>      struct empty {} e1;
>      double f0;
>      double f1;
> };

Then shouldn't we try to avoid the extra register in all cases, instead 
of wasting one regardless? ;-)

Re: [PATCH] LoongArch: Fix the problem of structure parameter passing in C++. This structure has empty structure members and less than three floating point members.

[Lulu Cheng]

在 2023/5/25 上午10:52, WANG Xuerui 写道:
>
> On 2023/5/25 10:46, Lulu Cheng wrote:
>>
>> 在 2023/5/25 上午4:15, Jason Merrill 写道:
>>> On Wed, May 24, 2023 at 5:00 AM Jonathan Wakely via Gcc-patches 
>>> <gcc-patches@gcc.gnu.org <mailto:gcc-patches@gcc.gnu.org>> wrote:
>>>
>>>     On Wed, 24 May 2023 at 09:41, Xi Ruoyao <xry111@xry111.site> wrote:
>>>
>>>     > Wang Lei raised some concerns about Itanium C++ ABI, so let's
>>>     ask a C++
>>>     > expert here...
>>>     >
>>>     > Jonathan: AFAIK the standard and the Itanium ABI treats an empty
>>>     class
>>>     > as size 1
>>>
>>>     Only as a complete object, not as a subobject.
>>>
>>>
>>> Also as a data member subobject.
>>>
>>>     > in order to guarantee unique address, so for the following:
>>>     >
>>>     > class Empty {};
>>>     > class Test { Empty empty; double a, b; };
>>>
>>>     There is no need to have a unique address here, so Test::empty and
>>>     Test::a
>>>     have the same address. It's a potentially-overlapping subobject.
>>>
>>>     For the Itanium ABI, sizeof(Test) == 2 * sizeof(double).
>>>
>>>
>>> That would be true if Test::empty were marked [[no_unique_address]], 
>>> but without that attribute, sizeof(Test) is actually 3 * 
>>> sizeof(double).
>>>
>>>     > When we pass "Test" via registers, we may only allocate the
>>>     registers
>>>     > for Test::a and Test::b, and complete ignore Test::empty because
>>>     there
>>>     > is no addresses of registers.  Is this correct or not?
>>>
>>>     I think that's a decision for the loongarch psABI. In principle,
>>>     there's no
>>>     reason a register has to be used to pass Test::empty, since you
>>>     can't read
>>>     from it or write to it.
>>>
>>>
>>> Agreed.  The Itanium C++ ABI has nothing to say about how registers 
>>> are allocated for parameter passing; this is a matter for the psABI.
>>>
>>> And there is no need for a psABI to allocate a register for 
>>> Test::empty because it contains no data.
>>>
>>> In the x86_64 psABI, Test above is passed in memory because of its 
>>> size ("the size of the aggregate exceeds two eightbytes...").  But
>>>
>>> struct Test2 { Empty empty; double a; };
>>>
>>> is passed in a single floating-point register; the Test2::empty 
>>> subobject is not passed anywhere, because its eightbyte is 
>>> classified as NO_CLASS, because there is no actual data there.
>>
>>
>>
>>>
>>> I know nothing about the LoongArch psABI, but going out of your way 
>>> to assign a register to an empty class seems like a mistake.
>>
>> MIPS64 and ARM64 also allocate parameter registers for empty structs. 
>> https://godbolt.org/z/jT4cY3T5o
>>
>> Our original intention is not to pass this empty structure member, 
>> but to make the following two structures treat empty structure members
>>
>> in the same way in the process of passing parameters.
>>
>> struct st1
>> {
>>      struct empty {} e1;
>>      long a;
>>      long b;
>> };
>>
>> struct st2
>> {
>>      struct empty {} e1;
>>      double f0;
>>      double f1;
>> };
>
> Then shouldn't we try to avoid the extra register in all cases, 
> instead of wasting one regardless? ;-)

https://godbolt.org/z/eK5T3Erbs

Compared with the situation of x86-64, if it is necessary not to pass 
empty structure members, it is difficult to achieve uniform processing.

Re: [PATCH] LoongArch: Fix the problem of structure parameter passing in C++. This structure has empty structure members and less than three floating point members.

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 3848 bytes --]

On Wed, 24 May 2023 at 21:15, Jason Merrill <jason@redhat.com> wrote:

> On Wed, May 24, 2023 at 5:00 AM Jonathan Wakely via Gcc-patches <
> gcc-patches@gcc.gnu.org> wrote:
>
>> On Wed, 24 May 2023 at 09:41, Xi Ruoyao <xry111@xry111.site> wrote:
>>
>> > Wang Lei raised some concerns about Itanium C++ ABI, so let's ask a C++
>> > expert here...
>> >
>> > Jonathan: AFAIK the standard and the Itanium ABI treats an empty class
>> > as size 1
>>
>> Only as a complete object, not as a subobject.
>>
>
> Also as a data member subobject.
>
>
>> > in order to guarantee unique address, so for the following:
>> >
>> > class Empty {};
>> > class Test { Empty empty; double a, b; };
>>
>> There is no need to have a unique address here, so Test::empty and Test::a
>> have the same address. It's a potentially-overlapping subobject.
>>
>> For the Itanium ABI, sizeof(Test) == 2 * sizeof(double).
>>
>
> That would be true if Test::empty were marked [[no_unique_address]], but
> without that attribute, sizeof(Test) is actually 3 * sizeof(double).
>

Doh, yes.


>
>
>> > When we pass "Test" via registers, we may only allocate the registers
>> > for Test::a and Test::b, and complete ignore Test::empty because there
>> > is no addresses of registers.  Is this correct or not?
>>
>> I think that's a decision for the loongarch psABI. In principle, there's
>> no
>> reason a register has to be used to pass Test::empty, since you can't read
>> from it or write to it.
>>
>
> Agreed.  The Itanium C++ ABI has nothing to say about how registers are
> allocated for parameter passing; this is a matter for the psABI.
>
> And there is no need for a psABI to allocate a register for Test::empty
> because it contains no data.
>
> In the x86_64 psABI, Test above is passed in memory because of its size
> ("the size of the aggregate exceeds two eightbytes...").  But
>
> struct Test2 { Empty empty; double a; };
>
> is passed in a single floating-point register; the Test2::empty subobject
> is not passed anywhere, because its eightbyte is classified as NO_CLASS,
> because there is no actual data there.
>
> I know nothing about the LoongArch psABI, but going out of your way to
> assign a register to an empty class seems like a mistake.
>
> > On Wed, 2023-05-24 at 14:45 +0800, Xi Ruoyao via Gcc-patches wrote:
>> > > On Wed, 2023-05-24 at 14:04 +0800, Lulu Cheng wrote:
>> > > > An empty struct type that is not non-trivial for the purposes of
>> > > > calls
>> > > > will be treated as though it were the following C type:
>> > > >
>> > > > struct {
>> > > >   char c;
>> > > > };
>> > > >
>> > > > Before this patch was added, a structure parameter containing an
>> > > > empty structure and
>> > > > less than three floating-point members was passed through one or two
>> > > > floating-point
>> > > > registers, while nested empty structures are ignored. Which did not
>> > > > conform to the
>> > > > calling convention.
>> > >
>> > > No, it's a deliberate decision I've made in
>> > > https://gcc.gnu.org/r12-8294.  And we already agreed "the ABI needs
>> to
>> > > be updated" when we applied r12-8294, but I've never improved my
>> > > English
>> > > skill to revise the ABI myself :(.
>> > >
>> > > We are also using the same "de-facto" ABI throwing away the empty
>> > > struct
>> > > for Clang++ (https://reviews.llvm.org/D132285).  So we should update
>> > > the
>> > > spec here, instead of changing every implementation.
>> > >
>> > > The C++ standard treats the empty struct as size 1 for ensuring the
>> > > semantics of pointer comparison operations.  When we pass it through
>> > > the
>> > > registers, there is no need to really consider the empty field because
>> > > there is no pointers to registers.
>> > >
>> >
>> >
>>
>>

Re: [PATCH] LoongArch: Fix the problem of structure parameter passing in C++. This structure has empty structure members and less than three floating point members.

[Lulu Cheng]

在 2023/5/24 下午2:45, Xi Ruoyao 写道:
> On Wed, 2023-05-24 at 14:04 +0800, Lulu Cheng wrote:
>> An empty struct type that is not non-trivial for the purposes of calls
>> will be treated as though it were the following C type:
>>
>> struct {
>>    char c;
>> };
>>
>> Before this patch was added, a structure parameter containing an empty structure and
>> less than three floating-point members was passed through one or two floating-point
>> registers, while nested empty structures are ignored. Which did not conform to the
>> calling convention.
> No, it's a deliberate decision I've made in
> https://gcc.gnu.org/r12-8294.  And we already agreed "the ABI needs to
> be updated" when we applied r12-8294, but I've never improved my English
> skill to revise the ABI myself :(.
>
> We are also using the same "de-facto" ABI throwing away the empty struct
> for Clang++ (https://reviews.llvm.org/D132285).  So we should update the
> spec here, instead of changing every implementation.
>
> The C++ standard treats the empty struct as size 1 for ensuring the
> semantics of pointer comparison operations.  When we pass it through the
> registers, there is no need to really consider the empty field because
> there is no pointers to registers.
>
I think that the rules for passing parameters to empty structures or 
nested empty structures should be unified,

but the current implementation in gcc is as follows(in C++):

Compare the two structures,the current implementation is as follows:

struct st1
{
   struct empty {} e1;
   long a;
   long b;
};

passed by reference.


struct st2
{
   struct empty {} e1;
   double f0;
   double f1;
};

passed through two floating-point registers.

Judging from the size of the structure, the size of st2 is already 
larger than 2xGRLEN, should be passed by reference.

Re: [PATCH] LoongArch: Fix the problem of structure parameter passing in C++. This structure has empty structure members and less than three floating point members.

[Xi Ruoyao]

On Wed, 2023-05-24 at 16:47 +0800, Lulu Cheng wrote:
> 
> 在 2023/5/24 下午2:45, Xi Ruoyao 写道:
> > On Wed, 2023-05-24 at 14:04 +0800, Lulu Cheng wrote:
> > > An empty struct type that is not non-trivial for the purposes of
> > > calls
> > > will be treated as though it were the following C type:
> > > 
> > > struct {
> > >    char c;
> > > };
> > > 
> > > Before this patch was added, a structure parameter containing an
> > > empty structure and
> > > less than three floating-point members was passed through one or
> > > two floating-point
> > > registers, while nested empty structures are ignored. Which did
> > > not conform to the
> > > calling convention.
> > No, it's a deliberate decision I've made in
> > https://gcc.gnu.org/r12-8294.  And we already agreed "the ABI needs
> > to
> > be updated" when we applied r12-8294, but I've never improved my
> > English
> > skill to revise the ABI myself :(.
> > 
> > We are also using the same "de-facto" ABI throwing away the empty
> > struct
> > for Clang++ (https://reviews.llvm.org/D132285).  So we should update
> > the
> > spec here, instead of changing every implementation.
> > 
> > The C++ standard treats the empty struct as size 1 for ensuring the
> > semantics of pointer comparison operations.  When we pass it through
> > the
> > registers, there is no need to really consider the empty field
> > because
> > there is no pointers to registers.
> > 
> I think that the rules for passing parameters to empty structures or 
> nested empty structures should be unified,

There is no need to unify them because "passing a struct" is already
different from "passing its members one by one".  Say:

int f1(int a, int b);

and

int f2(struct {int a, b;} ab);

"a" and "b" are already passed differently.

> but the current implementation in gcc is as follows(in C++):
> 
> Compare the two structures,the current implementation is as follows:
> 
> struct st1
> {
>    struct empty {} e1;
>    long a;
>    long b;
> };
> 
> passed by reference.
> 
> 
> struct st2
> {
>    struct empty {} e1;
>    double f0;
>    double f1;
> };
> 
> passed through two floating-point registers.

Well this is nasty, but it is the same behavior as RISC-V:
https://godbolt.org/z/fEexq148r

I deliberately made our logic similar to RISC-V in r12-8294 because
"there seems no reason to do it differently".  Maybe I was wrong and we
should have ignored st1::e1 as well (but IIRC we were running out of
time for GCC 12 release so we didn't have time to consider this :( ).

But now it's better to "keep the current behavior as-is" because:

1. The current behavior of GCC and Clang already matches and the
behavior is kept since the day one GCC and Clang supports LoongArch.  So
there is currently no ABI incompatibility in practice, but changing the
behavior will introduce an ABI incompatibility.
2. Changing the behavior will make the compiler more complex, and
slower.
3. Changing the behavior will need a -Wpsabi warning according to the
GCC policy, leading to more boring code (and more slow-down) in the
compiler.

> Judging from the size of the structure, the size of st2 is already 
> larger than 2xGRLEN, should be passed by reference.

I'd consider it a flaw in the psABI doc because it was obviously written
w/o considering the possibility of a zero-sized field in an aggregate.

I knew this flaw when I created r12-8294 and I planned to revise the
psABI doc for it, but I've never improved my English skill enough to do
the work.  I'm considering copying some word from RISC-V psABI if there
is no copyright issue.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] LoongArch: Fix the problem of structure parameter passing in C++. This structure has empty structure members and less than three floating point members.

[Lulu Cheng]

在 2023/5/24 下午5:25, Xi Ruoyao 写道:
> On Wed, 2023-05-24 at 16:47 +0800, Lulu Cheng wrote:
>> 在 2023/5/24 下午2:45, Xi Ruoyao 写道:
>>> On Wed, 2023-05-24 at 14:04 +0800, Lulu Cheng wrote:
>>>> An empty struct type that is not non-trivial for the purposes of
>>>> calls
>>>> will be treated as though it were the following C type:
>>>>
>>>> struct {
>>>>     char c;
>>>> };
>>>>
>>>> Before this patch was added, a structure parameter containing an
>>>> empty structure and
>>>> less than three floating-point members was passed through one or
>>>> two floating-point
>>>> registers, while nested empty structures are ignored. Which did
>>>> not conform to the
>>>> calling convention.
>>> No, it's a deliberate decision I've made in
>>> https://gcc.gnu.org/r12-8294.  And we already agreed "the ABI needs
>>> to
>>> be updated" when we applied r12-8294, but I've never improved my
>>> English
>>> skill to revise the ABI myself :(.
>>>
>>> We are also using the same "de-facto" ABI throwing away the empty
>>> struct
>>> for Clang++ (https://reviews.llvm.org/D132285).  So we should update
>>> the
>>> spec here, instead of changing every implementation.
>>>
>>> The C++ standard treats the empty struct as size 1 for ensuring the
>>> semantics of pointer comparison operations.  When we pass it through
>>> the
>>> registers, there is no need to really consider the empty field
>>> because
>>> there is no pointers to registers.
>>>
>> I think that the rules for passing parameters to empty structures or
>> nested empty structures should be unified,
> There is no need to unify them because "passing a struct" is already
> different from "passing its members one by one".  Say:
>
> int f1(int a, int b);
>
> and
>
> int f2(struct {int a, b;} ab);
>
> "a" and "b" are already passed differently.
I mean I think that empty structs in st1 and st2 should be treated the 
same way in the way of passing parameters.
>
>> but the current implementation in gcc is as follows(in C++):
>>
>> Compare the two structures,the current implementation is as follows:
>>
>> struct st1
>> {
>>     struct empty {} e1;
>>     long a;
>>     long b;
>> };
>>
>> passed by reference.
>>
>>
>> struct st2
>> {
>>     struct empty {} e1;
>>     double f0;
>>     double f1;
>> };
>>
>> passed through two floating-point registers.
> Well this is nasty, but it is the same behavior as RISC-V:
> https://godbolt.org/z/fEexq148r
>
> I deliberately made our logic similar to RISC-V in r12-8294 because
> "there seems no reason to do it differently".  Maybe I was wrong and we
> should have ignored st1::e1 as well (but IIRC we were running out of
> time for GCC 12 release so we didn't have time to consider this :( ).
>
> But now it's better to "keep the current behavior as-is" because:
>
> 1. The current behavior of GCC and Clang already matches and the
> behavior is kept since the day one GCC and Clang supports LoongArch.  So
> there is currently no ABI incompatibility in practice, but changing the
> behavior will introduce an ABI incompatibility.

The parameter passing rules for a single empty structure are different 
in GCC and Clang.

eg:

void test (struct empty, int a);

In GCC, the empty structure is passed through $a0, and the variable a is 
passed through $a1,

but Clang passes a through $a0, and the empty structure is ignored.

> 2. Changing the behavior will make the compiler more complex, and
> slower.
> 3. Changing the behavior will need a -Wpsabi warning according to the
> GCC policy, leading to more boring code (and more slow-down) in the
> compiler.

I really understand and thank you for your concerns, we have also 
considered the issue of compatibility.

Before the modification, we made an assessment. The colleagues of the 
operating system

built a total of 3,300 linux basic packages, and only one package was 
affected by this modification.

This is why GCC fixes this as a bug without adding -Wpsabi.

>
>> Judging from the size of the structure, the size of st2 is already
>> larger than 2xGRLEN, should be passed by reference.
> I'd consider it a flaw in the psABI doc because it was obviously written
> w/o considering the possibility of a zero-sized field in an aggregate.
>
> I knew this flaw when I created r12-8294 and I planned to revise the
> psABI doc for it, but I've never improved my English skill enough to do
> the work.  I'm considering copying some word from RISC-V psABI if there
> is no copyright issue.
>

Re: [PATCH] LoongArch: Fix the problem of structure parameter passing in C++. This structure has empty structure members and less than three floating point members.

[Xi Ruoyao]

On Wed, 2023-05-24 at 18:07 +0800, Lulu Cheng wrote:
> 
> 在 2023/5/24 下午5:25, Xi Ruoyao 写道:
> > On Wed, 2023-05-24 at 16:47 +0800, Lulu Cheng wrote:
> > > 在 2023/5/24 下午2:45, Xi Ruoyao 写道:
> > > > On Wed, 2023-05-24 at 14:04 +0800, Lulu Cheng wrote:
> > > > > An empty struct type that is not non-trivial for the purposes of
> > > > > calls
> > > > > will be treated as though it were the following C type:
> > > > > 
> > > > > struct {
> > > > >     char c;
> > > > > };
> > > > > 
> > > > > Before this patch was added, a structure parameter containing an
> > > > > empty structure and
> > > > > less than three floating-point members was passed through one or
> > > > > two floating-point
> > > > > registers, while nested empty structures are ignored. Which did
> > > > > not conform to the
> > > > > calling convention.
> > > > No, it's a deliberate decision I've made in
> > > > https://gcc.gnu.org/r12-8294.  And we already agreed "the ABI needs
> > > > to
> > > > be updated" when we applied r12-8294, but I've never improved my
> > > > English
> > > > skill to revise the ABI myself :(.
> > > > 
> > > > We are also using the same "de-facto" ABI throwing away the empty
> > > > struct
> > > > for Clang++ (https://reviews.llvm.org/D132285).  So we should update
> > > > the
> > > > spec here, instead of changing every implementation.
> > > > 
> > > > The C++ standard treats the empty struct as size 1 for ensuring the
> > > > semantics of pointer comparison operations.  When we pass it through
> > > > the
> > > > registers, there is no need to really consider the empty field
> > > > because
> > > > there is no pointers to registers.
> > > > 
> > > I think that the rules for passing parameters to empty structures or
> > > nested empty structures should be unified,
> > There is no need to unify them because "passing a struct" is already
> > different from "passing its members one by one".  Say:
> > 
> > int f1(int a, int b);
> > 
> > and
> > 
> > int f2(struct {int a, b;} ab);
> > 
> > "a" and "b" are already passed differently.
> I mean I think that empty structs in st1 and st2 should be treated the
> same way in the way of passing parameters.
> > 
> > > but the current implementation in gcc is as follows(in C++):
> > > 
> > > Compare the two structures,the current implementation is as follows:
> > > 
> > > struct st1
> > > {
> > >     struct empty {} e1;
> > >     long a;
> > >     long b;
> > > };
> > > 
> > > passed by reference.
> > > 
> > > 
> > > struct st2
> > > {
> > >     struct empty {} e1;
> > >     double f0;
> > >     double f1;
> > > };
> > > 
> > > passed through two floating-point registers.
> > Well this is nasty, but it is the same behavior as RISC-V:
> > https://godbolt.org/z/fEexq148r
> > 
> > I deliberately made our logic similar to RISC-V in r12-8294 because
> > "there seems no reason to do it differently".  Maybe I was wrong and we
> > should have ignored st1::e1 as well (but IIRC we were running out of
> > time for GCC 12 release so we didn't have time to consider this :( ).
> > 
> > But now it's better to "keep the current behavior as-is" because:
> > 
> > 1. The current behavior of GCC and Clang already matches and the
> > behavior is kept since the day one GCC and Clang supports LoongArch.  So
> > there is currently no ABI incompatibility in practice, but changing the
> > behavior will introduce an ABI incompatibility.
> 
> The parameter passing rules for a single empty structure are different
> in GCC and Clang.
> 
> eg:
> 
> void test (struct empty, int a);
> 
> In GCC, the empty structure is passed through $a0, and the variable a is 
> passed through $a1,
> 
> but Clang passes a through $a0, and the empty structure is ignored.
> 
> > 2. Changing the behavior will make the compiler more complex, and
> > slower.
> > 3. Changing the behavior will need a -Wpsabi warning according to the
> > GCC policy, leading to more boring code (and more slow-down) in the
> > compiler.
> 
> I really understand and thank you for your concerns, we have also 
> considered the issue of compatibility.
> 
> Before the modification, we made an assessment. The colleagues of the 
> operating system
> 
> built a total of 3,300 linux basic packages, and only one package was 
> affected by this modification.
> 
> This is why GCC fixes this as a bug without adding -Wpsabi.

If you are really determined to do this, then OK.  I'm in a very bad
mood and I don't want to spend my mental strength on debating (esp. on a
corner case unlikely to affect "real" code) anymore.

But remember to add a entry in GCC 14 changes.html, and test this thing:

struct Empty {};

struct Something : Empty
{
  double a, b;
};

If we are not careful enough we may introduce a ABI mismatch between -
std=c++14 and -std=c++17 here.  See https://gcc.gnu.org/PR94383.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University