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 <chenglulu@loongson.cn>]

在 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.
>