Re: [PATCH] gcc/riscv: Correctly ignore empty C++ structs when flattening for ABI
On Wed, Mar 13, 2019 at 11:39 AM Andrew Burgess wrote: > gcc/ChangeLog: > > PR target/89627 > * config/riscv/riscv.c (riscv_pass_fpr_single): Add offset > parameter, and make use of it. > (riscv_get_arg_info): Pass offset to riscv_pass_fpr_single. > > gcc/testsuite/ChangeLog: > > PR target/89627 > * g++.target/riscv/call-with-empty-struct-float.C: New file. > * g++.target/riscv/call-with-empty-struct-int.C: New file. > * g++.target/riscv/call-with-empty-struct.H: New file. > * g++.target/riscv/riscv.exp: New file. This is OK. We can always move the testcases later if that is useful. Jim
Re: [PATCH] gcc/riscv: Correctly ignore empty C++ structs when flattening for ABI
On Wed, Mar 13, 2019 at 11:53 AM Andrew Pinski wrote: > This testcase seems generic, that is it has no ABI dependent values > attached to it. > Can it be moved to a more generic location instead? Maybe there are > other targets which get this incorrect also. I'm not aware of any other target that passes structs the same way. RISC-V fails the testcase because it will decompose a struct containing only floats into fields and pass the fields as scalars. IA-64 and Aarch64 have support for HFA (homogenous FP aggregates), but this case doesn't qualify as an HFA because of the empty struct, and hence this case gets no special treatment for those targets and isn't broken. Jim
Re: [PATCH] gcc/riscv: Correctly ignore empty C++ structs when flattening for ABI
On Wed, Mar 13, 2019 at 11:40 AM Andrew Burgess
wrote:
>
> Jim,
>
> Sorry for the delay in sending this patch.
>
> Thanks,
> Andrew
>
> ---
>
> This fixes PR target/89627.
>
> The RISC-V ABI document[1] says:
>
>For the purposes of this section, "struct" refers to a C struct
>with its hierarchy flattened, including any array fields. That is,
>struct { struct { float f[1]; } g[2]; } and struct { float f; float
>g; } are treated the same. Fields containing empty structs or
>unions are ignored while flattening, even in C++, unless they have
>nontrivial copy constructors or destructors.
>
> However, this flattening only applies when one of the fields of the
> flattened structure can be placed into a floating point register,
> otherwise no flattening occurs.
>
> Currently GCC fails to correctly consider that empty C++ structures
> have a non-zero size when constructing the arguments from a flattened
> structure, and as a result, trying to pass a C++ structure like this:
>
> struct sf { struct {} e; float f; };
>
> Doesn't work correctly, GCC fails to take the offset of 'f' within
> 'sf' into account and will actually pass the space backing 'e' as the
> contents of 'f'.
>
> This patch fixes this so that 'f' will be passed correctly. A couple
> of new tests are added to cover this functionality.
>
> [1] https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md
>
> gcc/ChangeLog:
>
> PR target/89627
> * config/riscv/riscv.c (riscv_pass_fpr_single): Add offset
> parameter, and make use of it.
> (riscv_get_arg_info): Pass offset to riscv_pass_fpr_single.
>
> gcc/testsuite/ChangeLog:
>
> PR target/89627
> * g++.target/riscv/call-with-empty-struct-float.C: New file.
> * g++.target/riscv/call-with-empty-struct-int.C: New file.
> * g++.target/riscv/call-with-empty-struct.H: New file.
> * g++.target/riscv/riscv.exp: New file.
This testcase seems generic, that is it has no ABI dependent values
attached to it.
Can it be moved to a more generic location instead? Maybe there are
other targets which get this incorrect also.
Thanks,
Andrew Pinski
> ---
> gcc/ChangeLog | 7 +
> gcc/config/riscv/riscv.c | 8 +++--
> gcc/testsuite/ChangeLog| 8 +
> .../riscv/call-with-empty-struct-float.C | 6
> .../g++.target/riscv/call-with-empty-struct-int.C | 6
> .../g++.target/riscv/call-with-empty-struct.H | 19
> gcc/testsuite/g++.target/riscv/riscv.exp | 34
> ++
> 7 files changed, 85 insertions(+), 3 deletions(-)
> create mode 100644
> gcc/testsuite/g++.target/riscv/call-with-empty-struct-float.C
> create mode 100644
> gcc/testsuite/g++.target/riscv/call-with-empty-struct-int.C
> create mode 100644 gcc/testsuite/g++.target/riscv/call-with-empty-struct.H
> create mode 100644 gcc/testsuite/g++.target/riscv/riscv.exp
>
> diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c
> index 8881f80e18f..8e78ab76375 100644
> --- a/gcc/config/riscv/riscv.c
> +++ b/gcc/config/riscv/riscv.c
> @@ -2449,13 +2449,14 @@ riscv_pass_aggregate_in_fpr_and_gpr_p (const_tree
> type,
>
> static rtx
> riscv_pass_fpr_single (machine_mode type_mode, unsigned regno,
> - machine_mode value_mode)
> + machine_mode value_mode,
> + HOST_WIDE_INT offset)
> {
>rtx x = gen_rtx_REG (value_mode, regno);
>
>if (type_mode != value_mode)
> {
> - x = gen_rtx_EXPR_LIST (VOIDmode, x, const0_rtx);
> + x = gen_rtx_EXPR_LIST (VOIDmode, x, GEN_INT (offset));
>x = gen_rtx_PARALLEL (type_mode, gen_rtvec (1, x));
> }
>return x;
> @@ -2517,7 +2518,8 @@ riscv_get_arg_info (struct riscv_arg_info *info, const
> CUMULATIVE_ARGS *cum,
> {
> case 1:
> return riscv_pass_fpr_single (mode, fregno,
> - TYPE_MODE (fields[0].type));
> + TYPE_MODE (fields[0].type),
> + fields[0].offset);
>
> case 2:
> return riscv_pass_fpr_pair (mode, fregno,
> diff --git a/gcc/testsuite/g++.target/riscv/call-with-empty-struct-float.C
> b/gcc/testsuite/g++.target/riscv/call-with-empty-struct-float.C
> new file mode 100644
> index 000..76d0dc6d93d
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/riscv/call-with-empty-struct-float.C
> @@ -0,0 +1,6 @@
> +// { dg-do run }
> +
> +#include "call-with-empty-struct.H"
> +
> +MAKE_STRUCT_PASSING_TEST(float,2.5)
> +
> diff --git a/gcc/testsuite/g++.target/riscv/call-with-empty-struct-int.C
> b/gcc/testsuite/g++.target/riscv/call-with-empty-struct-int.C
> new file mode 100644
> index 000..cc82912dc3e
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/riscv/call-with-empty-struct-int.C
[PATCH] gcc/riscv: Correctly ignore empty C++ structs when flattening for ABI
Jim,
Sorry for the delay in sending this patch.
Thanks,
Andrew
---
This fixes PR target/89627.
The RISC-V ABI document[1] says:
For the purposes of this section, "struct" refers to a C struct
with its hierarchy flattened, including any array fields. That is,
struct { struct { float f[1]; } g[2]; } and struct { float f; float
g; } are treated the same. Fields containing empty structs or
unions are ignored while flattening, even in C++, unless they have
nontrivial copy constructors or destructors.
However, this flattening only applies when one of the fields of the
flattened structure can be placed into a floating point register,
otherwise no flattening occurs.
Currently GCC fails to correctly consider that empty C++ structures
have a non-zero size when constructing the arguments from a flattened
structure, and as a result, trying to pass a C++ structure like this:
struct sf { struct {} e; float f; };
Doesn't work correctly, GCC fails to take the offset of 'f' within
'sf' into account and will actually pass the space backing 'e' as the
contents of 'f'.
This patch fixes this so that 'f' will be passed correctly. A couple
of new tests are added to cover this functionality.
[1] https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md
gcc/ChangeLog:
PR target/89627
* config/riscv/riscv.c (riscv_pass_fpr_single): Add offset
parameter, and make use of it.
(riscv_get_arg_info): Pass offset to riscv_pass_fpr_single.
gcc/testsuite/ChangeLog:
PR target/89627
* g++.target/riscv/call-with-empty-struct-float.C: New file.
* g++.target/riscv/call-with-empty-struct-int.C: New file.
* g++.target/riscv/call-with-empty-struct.H: New file.
* g++.target/riscv/riscv.exp: New file.
---
gcc/ChangeLog | 7 +
gcc/config/riscv/riscv.c | 8 +++--
gcc/testsuite/ChangeLog| 8 +
.../riscv/call-with-empty-struct-float.C | 6
.../g++.target/riscv/call-with-empty-struct-int.C | 6
.../g++.target/riscv/call-with-empty-struct.H | 19
gcc/testsuite/g++.target/riscv/riscv.exp | 34 ++
7 files changed, 85 insertions(+), 3 deletions(-)
create mode 100644
gcc/testsuite/g++.target/riscv/call-with-empty-struct-float.C
create mode 100644 gcc/testsuite/g++.target/riscv/call-with-empty-struct-int.C
create mode 100644 gcc/testsuite/g++.target/riscv/call-with-empty-struct.H
create mode 100644 gcc/testsuite/g++.target/riscv/riscv.exp
diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c
index 8881f80e18f..8e78ab76375 100644
--- a/gcc/config/riscv/riscv.c
+++ b/gcc/config/riscv/riscv.c
@@ -2449,13 +2449,14 @@ riscv_pass_aggregate_in_fpr_and_gpr_p (const_tree type,
static rtx
riscv_pass_fpr_single (machine_mode type_mode, unsigned regno,
- machine_mode value_mode)
+ machine_mode value_mode,
+ HOST_WIDE_INT offset)
{
rtx x = gen_rtx_REG (value_mode, regno);
if (type_mode != value_mode)
{
- x = gen_rtx_EXPR_LIST (VOIDmode, x, const0_rtx);
+ x = gen_rtx_EXPR_LIST (VOIDmode, x, GEN_INT (offset));
x = gen_rtx_PARALLEL (type_mode, gen_rtvec (1, x));
}
return x;
@@ -2517,7 +2518,8 @@ riscv_get_arg_info (struct riscv_arg_info *info, const
CUMULATIVE_ARGS *cum,
{
case 1:
return riscv_pass_fpr_single (mode, fregno,
- TYPE_MODE (fields[0].type));
+ TYPE_MODE (fields[0].type),
+ fields[0].offset);
case 2:
return riscv_pass_fpr_pair (mode, fregno,
diff --git a/gcc/testsuite/g++.target/riscv/call-with-empty-struct-float.C
b/gcc/testsuite/g++.target/riscv/call-with-empty-struct-float.C
new file mode 100644
index 000..76d0dc6d93d
--- /dev/null
+++ b/gcc/testsuite/g++.target/riscv/call-with-empty-struct-float.C
@@ -0,0 +1,6 @@
+// { dg-do run }
+
+#include "call-with-empty-struct.H"
+
+MAKE_STRUCT_PASSING_TEST(float,2.5)
+
diff --git a/gcc/testsuite/g++.target/riscv/call-with-empty-struct-int.C
b/gcc/testsuite/g++.target/riscv/call-with-empty-struct-int.C
new file mode 100644
index 000..cc82912dc3e
--- /dev/null
+++ b/gcc/testsuite/g++.target/riscv/call-with-empty-struct-int.C
@@ -0,0 +1,6 @@
+/* { dg-do run } */
+
+#include "call-with-empty-struct.H"
+
+MAKE_STRUCT_PASSING_TEST(int,2)
+
diff --git a/gcc/testsuite/g++.target/riscv/call-with-empty-struct.H
b/gcc/testsuite/g++.target/riscv/call-with-empty-struct.H
new file mode 100644
index 000..d2a46e78619
--- /dev/null
+++ b/gcc/testsuite/g++.target/riscv/call-with-empty-struct.H
@@ -0,0 +1,19 @@
+#define MAKE_STRUCT_PASSING_TEST(type,val) \
+ static struct struct_ ## type ## _t
