[RFA] expand: empty class return optimization [PR88529]

Message ID 20210615032422.1910808-1-jason@redhat.com
State New
Headers show
Series
  • [RFA] expand: empty class return optimization [PR88529]
Related show

Commit Message

Aldy Hernandez via Gcc-patches June 15, 2021, 3:24 a.m.
The x86_64 psABI says that an empty class isn't passed or returned in memory or
registers, so we shouldn't set %eax in this function.  Is this a reasonable
place to implement that?  Another possibility would be to remove the hack to
prevent i386.c:function_value_64 from returning NULL in this case and fix the
callers to deal, but that seems like more work.

The df-scan hunk catches the case where we look at a 0-length reg and build
a range the length of unsigned int, which happened before I changed
assign_parms to match expand_function_end.

Tested x86_64-pc-linux-gnu.  OK for trunk?

	PR target/88529

gcc/ChangeLog:

	* df-scan.c (df_ref_record): Check that regno < endregno.
	* function.c (assign_parms, expand_function_end): Do nothing with a
	TYPE_EMPTY_P result.

gcc/testsuite/ChangeLog:

	* g++.target/i386/empty-class1.C: New test.
---
 gcc/df-scan.c                                |  2 ++
 gcc/function.c                               | 16 ++++++++++------
 gcc/testsuite/g++.target/i386/empty-class1.C |  9 +++++++++
 3 files changed, 21 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/g++.target/i386/empty-class1.C


base-commit: 08ce1f4c5091b80b680d15c53a17237544a3cca8
prerequisite-patch-id: 320d08bee3615919d0cf6a9d33bc2247aece51c7
-- 
2.27.0

Comments

Aldy Hernandez via Gcc-patches June 21, 2021, 7:40 a.m. | #1
On Tue, Jun 15, 2021 at 5:25 AM Jason Merrill via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>

> The x86_64 psABI says that an empty class isn't passed or returned in memory or

> registers, so we shouldn't set %eax in this function.  Is this a reasonable

> place to implement that?  Another possibility would be to remove the hack to

> prevent i386.c:function_value_64 from returning NULL in this case and fix the

> callers to deal, but that seems like more work.

>

> The df-scan hunk catches the case where we look at a 0-length reg and build

> a range the length of unsigned int, which happened before I changed

> assign_parms to match expand_function_end.

>

> Tested x86_64-pc-linux-gnu.  OK for trunk?


OK.

Thanks,
Richard.

>         PR target/88529

>

> gcc/ChangeLog:

>

>         * df-scan.c (df_ref_record): Check that regno < endregno.

>         * function.c (assign_parms, expand_function_end): Do nothing with a

>         TYPE_EMPTY_P result.

>

> gcc/testsuite/ChangeLog:

>

>         * g++.target/i386/empty-class1.C: New test.

> ---

>  gcc/df-scan.c                                |  2 ++

>  gcc/function.c                               | 16 ++++++++++------

>  gcc/testsuite/g++.target/i386/empty-class1.C |  9 +++++++++

>  3 files changed, 21 insertions(+), 6 deletions(-)

>  create mode 100644 gcc/testsuite/g++.target/i386/empty-class1.C

>

> diff --git a/gcc/df-scan.c b/gcc/df-scan.c

> index 1268536b3f0..e9da64ff3df 100644

> --- a/gcc/df-scan.c

> +++ b/gcc/df-scan.c

> @@ -2595,6 +2595,8 @@ df_ref_record (enum df_ref_class cl,

>             ref_flags |= DF_REF_PARTIAL;

>           ref_flags |= DF_REF_MW_HARDREG;

>

> +         gcc_assert (regno < endregno);

> +

>           hardreg = problem_data->mw_reg_pool->allocate ();

>           hardreg->type = ref_type;

>           hardreg->flags = ref_flags;

> diff --git a/gcc/function.c b/gcc/function.c

> index 67576950983..6abaf3d116f 100644

> --- a/gcc/function.c

> +++ b/gcc/function.c

> @@ -3821,9 +3821,11 @@ assign_parms (tree fndecl)

>        tree decl_result = DECL_RESULT (fndecl);

>        rtx decl_rtl = DECL_RTL (decl_result);

>

> -      if (REG_P (decl_rtl)

> -         ? REGNO (decl_rtl) >= FIRST_PSEUDO_REGISTER

> -         : DECL_REGISTER (decl_result))

> +      if ((REG_P (decl_rtl)

> +          ? REGNO (decl_rtl) >= FIRST_PSEUDO_REGISTER

> +          : DECL_REGISTER (decl_result))

> +         /* Unless the psABI says not to.  */

> +         && !TYPE_EMPTY_P (TREE_TYPE (decl_result)))

>         {

>           rtx real_decl_rtl;

>

> @@ -5410,9 +5412,11 @@ expand_function_end (void)

>        tree decl_result = DECL_RESULT (current_function_decl);

>        rtx decl_rtl = DECL_RTL (decl_result);

>

> -      if (REG_P (decl_rtl)

> -         ? REGNO (decl_rtl) >= FIRST_PSEUDO_REGISTER

> -         : DECL_REGISTER (decl_result))

> +      if ((REG_P (decl_rtl)

> +          ? REGNO (decl_rtl) >= FIRST_PSEUDO_REGISTER

> +          : DECL_REGISTER (decl_result))

> +         /* Unless the psABI says not to.  */

> +         && !TYPE_EMPTY_P (TREE_TYPE (decl_result)))

>         {

>           rtx real_decl_rtl = crtl->return_rtx;

>           complex_mode cmode;

> diff --git a/gcc/testsuite/g++.target/i386/empty-class1.C b/gcc/testsuite/g++.target/i386/empty-class1.C

> new file mode 100644

> index 00000000000..c1992772d26

> --- /dev/null

> +++ b/gcc/testsuite/g++.target/i386/empty-class1.C

> @@ -0,0 +1,9 @@

> +// PR target/88529

> +// { dg-do compile { target { c++11 && x86_64-*-* } } }

> +// { dg-additional-options -fdump-rtl-expand }

> +// { dg-final { scan-rtl-dump-not "set" "expand" } }

> +// The x86_64 psABI says that f() doesn't put the return value anywhere.

> +

> +class A{};

> +

> +A f() { return {}; }

>

> base-commit: 08ce1f4c5091b80b680d15c53a17237544a3cca8

> prerequisite-patch-id: 320d08bee3615919d0cf6a9d33bc2247aece51c7

> --

> 2.27.0

>
Joseph Myers June 22, 2021, 9:18 p.m. | #2
This introduces an ICE building libgomp for ColdFire, as detected by my 
glibc bot.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101170

-- 
Joseph S. Myers
joseph@codesourcery.com

Patch

diff --git a/gcc/df-scan.c b/gcc/df-scan.c
index 1268536b3f0..e9da64ff3df 100644
--- a/gcc/df-scan.c
+++ b/gcc/df-scan.c
@@ -2595,6 +2595,8 @@  df_ref_record (enum df_ref_class cl,
 	    ref_flags |= DF_REF_PARTIAL;
 	  ref_flags |= DF_REF_MW_HARDREG;
 
+	  gcc_assert (regno < endregno);
+
 	  hardreg = problem_data->mw_reg_pool->allocate ();
 	  hardreg->type = ref_type;
 	  hardreg->flags = ref_flags;
diff --git a/gcc/function.c b/gcc/function.c
index 67576950983..6abaf3d116f 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -3821,9 +3821,11 @@  assign_parms (tree fndecl)
       tree decl_result = DECL_RESULT (fndecl);
       rtx decl_rtl = DECL_RTL (decl_result);
 
-      if (REG_P (decl_rtl)
-	  ? REGNO (decl_rtl) >= FIRST_PSEUDO_REGISTER
-	  : DECL_REGISTER (decl_result))
+      if ((REG_P (decl_rtl)
+	   ? REGNO (decl_rtl) >= FIRST_PSEUDO_REGISTER
+	   : DECL_REGISTER (decl_result))
+	  /* Unless the psABI says not to.  */
+	  && !TYPE_EMPTY_P (TREE_TYPE (decl_result)))
 	{
 	  rtx real_decl_rtl;
 
@@ -5410,9 +5412,11 @@  expand_function_end (void)
       tree decl_result = DECL_RESULT (current_function_decl);
       rtx decl_rtl = DECL_RTL (decl_result);
 
-      if (REG_P (decl_rtl)
-	  ? REGNO (decl_rtl) >= FIRST_PSEUDO_REGISTER
-	  : DECL_REGISTER (decl_result))
+      if ((REG_P (decl_rtl)
+	   ? REGNO (decl_rtl) >= FIRST_PSEUDO_REGISTER
+	   : DECL_REGISTER (decl_result))
+	  /* Unless the psABI says not to.  */
+	  && !TYPE_EMPTY_P (TREE_TYPE (decl_result)))
 	{
 	  rtx real_decl_rtl = crtl->return_rtx;
 	  complex_mode cmode;
diff --git a/gcc/testsuite/g++.target/i386/empty-class1.C b/gcc/testsuite/g++.target/i386/empty-class1.C
new file mode 100644
index 00000000000..c1992772d26
--- /dev/null
+++ b/gcc/testsuite/g++.target/i386/empty-class1.C
@@ -0,0 +1,9 @@ 
+// PR target/88529
+// { dg-do compile { target { c++11 && x86_64-*-* } } }
+// { dg-additional-options -fdump-rtl-expand }
+// { dg-final { scan-rtl-dump-not "set" "expand" } }
+// The x86_64 psABI says that f() doesn't put the return value anywhere.
+
+class A{};
+
+A f() { return {}; }