Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarification of rules for flattening structs containing arrays of empty records #358

Closed
asb opened this issue Jan 23, 2023 · 3 comments · Fixed by #365
Closed

Clarification of rules for flattening structs containing arrays of empty records #358

asb opened this issue Jan 23, 2023 · 3 comments · Fixed by #365

Comments

@asb
Copy link
Collaborator

asb commented Jan 23, 2023

The hard FP calling convention currently states "Fields containing empty structs or unions are ignored while flattening, even in C++, unless they have nontrivial copy constructors or destructors." It appears that g++ doesn't ignore an array of empty records in a struct. If this is indeed the desired behaviour, it would be good to document it.

$ cat t.c
struct empty { struct { struct { } e; }; };
struct s { struct empty e[1]; float f; };

float test_s(struct s a) {
  return a.f+1.0;
}

In the above example, the float is passed in a0 with g++ but fa0 with gcc.

@asb
Copy link
Collaborator Author

asb commented Jan 24, 2023

However zero-sized arrays of empty records are ignored by both g++ and gcc for flattening (thanks @luismarques for the suggestion to check that case!). I'm attempting to align Clang/LLVM with this behaviour in https://reviews.llvm.org/D142327

@nick-knight
Copy link
Contributor

nick-knight commented Jan 24, 2023

My interpretation is that s::e is, in the language of our spec, a "field containing an empty struct". So I would expect g++ to pass the float in fa0. Whether or not my interpretation is correct, I agree a clarification would be useful.

I'm not sure it is helpful to this discussion, but I believe that empty structs are a GNU C extension, while being (standard) C++; additionally, zero-length arrays are a GNU C/C++ extension. It's not clear to me what our psABI is expected to say about language extensions.

@kito-cheng
Copy link
Collaborator

ack, I plan to dig more deeper on lang spec in next few days, and seems sizeof (struct s) has different value in C++ or C mode for both GCC and clang, and study how other ABI handle this.

kito-cheng added a commit that referenced this issue Feb 10, 2023
Fix #358

The key point of this issue is C and C++ having different type size for
array of empty struct or union, size of empty struct or union is 0 for
C, and 1 for C++, and the size of array is type size multiplied by the
length of the array.

Also checked AArch64 code gen, they also generated different code between C
and C++, and ABI isn't explicitly describe this but has describe the size
of array is type * length.
kito-cheng added a commit that referenced this issue Feb 16, 2023
Fix #358

The key point of this issue is C and C++ having different type size for
array of empty struct or union, size of empty struct or union is 0 for
C, and 1 for C++, and the size of array is type size multiplied by the
length of the array.

Also checked AArch64 code gen, they also generated different code between C
and C++, and ABI isn't explicitly describe this but has describe the size
of array is type * length.
kito-cheng added a commit that referenced this issue Feb 16, 2023
Fix #358

The key point of this issue is C and C++ having different type size for
array of empty struct or union, size of empty struct or union is 0 for
C, and 1 for C++, and the size of array is type size multiplied by the
length of the array.

Also checked AArch64 code gen, they also generated different code between C
and C++, and ABI isn't explicitly describe this but has describe the size
of array is type * length.
kito-cheng added a commit that referenced this issue Feb 20, 2023
Fix #358

The key point of this issue is C and C++ having different type size for
array of empty struct or union, size of empty struct or union is 0 for
C, and 1 for C++, and the size of array is type size multiplied by the
length of the array.

Also checked AArch64 code gen, they also generated different code between C
and C++, and ABI isn't explicitly describe this but has describe the size
of array is type * length.
kito-cheng added a commit that referenced this issue Mar 23, 2023
Fix #358

The key point of this issue is C and C++ having different type size for
array of empty struct or union, size of empty struct or union is 0 for
C, and 1 for C++, and the size of array is type size multiplied by the
length of the array.

Also checked AArch64 code gen, they also generated different code between C
and C++, and ABI isn't explicitly describe this but has describe the size
of array is type * length.
kito-cheng added a commit that referenced this issue Mar 30, 2023
Fix #358

The key point of this issue is C and C++ having different type size for
array of empty struct or union, size of empty struct or union is 0 for
C, and 1 for C++, and the size of array is type size multiplied by the
length of the array.

Also checked AArch64 code gen, they also generated different code between C
and C++, and ABI isn't explicitly describe this but has describe the size
of array is type * length.
asb added a commit to llvm/llvm-project that referenced this issue Jul 24, 2023
… conventions in C++

As reported in <#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'). See also the relevant psABI issue
<riscv-non-isa/riscv-elf-psabi-doc#358> which
seeks to clarify the documentation.

Fixes #58929

Differential Revision: https://reviews.llvm.org/D142327
asb added a commit to llvm/llvm-project that referenced this issue Aug 7, 2023
…calling conventions in C++

As reported in <#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
<riscv-non-isa/riscv-elf-psabi-doc#358> seeks
to clarify this.

This patch was previously committed but reverted after a bug was found.
This recommit adds additional logic to prevent that bug (adding an extra
check for when a candidate from detectFPCCEligibleStructHelper may not
be valid).

Differential Revision: https://reviews.llvm.org/D142327
llvmbot pushed a commit to llvm/llvm-project-release-prs that referenced this issue Aug 7, 2023
…calling conventions in C++

As reported in <llvm/llvm-project#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
<riscv-non-isa/riscv-elf-psabi-doc#358> seeks
to clarify this.

This patch was previously committed but reverted after a bug was found.
This recommit adds additional logic to prevent that bug (adding an extra
check for when a candidate from detectFPCCEligibleStructHelper may not
be valid).

Differential Revision: https://reviews.llvm.org/D142327
tru pushed a commit to llvm/llvm-project-release-prs that referenced this issue Aug 7, 2023
…calling conventions in C++

As reported in <llvm/llvm-project#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
<riscv-non-isa/riscv-elf-psabi-doc#358> seeks
to clarify this.

This patch was previously committed but reverted after a bug was found.
This recommit adds additional logic to prevent that bug (adding an extra
check for when a candidate from detectFPCCEligibleStructHelper may not
be valid).

Differential Revision: https://reviews.llvm.org/D142327
veselypeta pushed a commit to veselypeta/cherillvm that referenced this issue Sep 7, 2024
… conventions in C++

As reported in <llvm/llvm-project#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'). See also the relevant psABI issue
<riscv-non-isa/riscv-elf-psabi-doc#358> which
seeks to clarify the documentation.

Fixes llvm/llvm-project#58929

Differential Revision: https://reviews.llvm.org/D142327
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants