diff mbox series

[IRA] Revert 11b8091fb to fix Bug 93221

Message ID 586c7e2d-5305-7bd2-8d57-85693feffb16@arm.com
State Superseded
Headers show
Series [IRA] Revert 11b8091fb to fix Bug 93221 | expand

Commit Message

Joel Jan. 21, 2020, 5:20 p.m. UTC
Hi all,

A previous change to simplify LRA introduced in 11b809 (From-SVN: 
r279550) disabled hard register splitting for -O0. This causes a problem 
on aarch64 in cases where parameters are passed in multiple registers 
(in the bug report an OI passed in 2 V4SI registers). This is mandated 
by the AAPCS.

Vlad, Eric, do you have a preferred alternate solution to reverting the 
patch?

Previously discussed here: 
https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01414.html
Bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93221

Bootstrapped and regression tested on aarch64

Changelog:

2020-01-21  Joel Hutton  <Joel.Hutton@arm.com\>

         * ira.c (ira): Revert use of simplified LRA algorithm.

gcc/testsuite/ChangeLog:

2020-01-21  Joel Hutton  <Joel.Hutton@arm.com\>

         PR bug/93221
         * gcc.target/aarch64/pr93221.c: New test.

Comments

Vladimir Makarov Jan. 21, 2020, 7:16 p.m. UTC | #1
On 1/21/20 12:20 PM, Joel Hutton wrote:
> Hi all,

>

> A previous change to simplify LRA introduced in 11b809 (From-SVN:

> r279550) disabled hard register splitting for -O0. This causes a problem

> on aarch64 in cases where parameters are passed in multiple registers

> (in the bug report an OI passed in 2 V4SI registers). This is mandated

> by the AAPCS.


I see.  I suspected that it can cause the problem but I thought the 
probability is very small for this.  I was wrong.  We should revert it 
at least for now.  Correct work of GCC is more important than saving 
cycles in -O0 mode.

> Vlad, Eric, do you have a preferred alternate solution to reverting the

> patch?

I am in favour of reverting the patch now.  But may be Eric can provide 
another version of the patch not causing the arm problem.  I am ready to 
reconsider this too.  So I guess the decision is upto Eric.
> Previously discussed here:

> https://gcc.gnu.org/ml/gcc-patches/2019-12/msg01414.html

> Bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93221

>

> Bootstrapped and regression tested on aarch64

>

> Changelog:

>

> 2020-01-21  Joel Hutton  <Joel.Hutton@arm.com\>

>

>           * ira.c (ira): Revert use of simplified LRA algorithm.

>

> gcc/testsuite/ChangeLog:

>

> 2020-01-21  Joel Hutton  <Joel.Hutton@arm.com\>

>

>           PR bug/93221

>           * gcc.target/aarch64/pr93221.c: New test.

>
Jakub Jelinek Jan. 21, 2020, 7:26 p.m. UTC | #2
On Tue, Jan 21, 2020 at 05:20:51PM +0000, Joel Hutton wrote:
> 2020-01-21  Joel Hutton  <Joel.Hutton@arm.com\>

> 

>          * ira.c (ira): Revert use of simplified LRA algorithm.

> 

> gcc/testsuite/ChangeLog:

> 

> 2020-01-21  Joel Hutton  <Joel.Hutton@arm.com\>

> 

>          PR bug/93221

>          * gcc.target/aarch64/pr93221.c: New test.


Not a review, just nitpicking.  Please avoid the backslash before >.
The PR number should go to both gcc/ and gcc/testsuite/ ChangeLog
entry and we don't have bug/ category; the bug is target category,
so it should be PR target/93221, or could be reclassified first in
bugzilla e.g. to middle-end and then written as PR middle-end/93221.

> @@ -0,0 +1,10 @@

> +/* PR bug/93221 */


Likewise here.

> +/* { dg-do compile } */

> +/* { dg-options "-O0 -mno-omit-leaf-frame-pointer" } */

> +

> +struct S { __Int32x4_t b[2]; };

> +

> +void __attribute__((optimize (0)))


Not sure if I understand why do you need optimize (0) attribute when the
whole test is compiled with -O0.  Doesn't it ICE without the attribute
too?

> +foo (struct S x)

> +{

> +}


	Jakub
Joel Jan. 21, 2020, 8:16 p.m. UTC | #3
On 21/01/2020 19:26, Jakub Jelinek wrote:
> Not a review, just nitpicking.  Please avoid the backslash before >.

> The PR number should go to both gcc/ and gcc/testsuite/ ChangeLog

> entry and we don't have bug/ category; the bug is target category,

> so it should be PR target/93221, or could be reclassified first in

> bugzilla e.g. to middle-end and then written as PR middle-end/93221.

>

Done
>> @@ -0,0 +1,10 @@

>> +/* PR bug/93221 */

> Likewise here.

Done
> Not sure if I understand why do you need optimize (0) attribute when the

> whole test is compiled with -O0.  Doesn't it ICE without the attribute

> too?

Done. It's not really necessary, belt and braces.

Updated patch attached
From 1a2980ef6eeb76dbf0556f806a85a4f49ad3ebdd Mon Sep 17 00:00:00 2001
From: Joel Hutton <Joel.Hutton@arm.com>

Date: Tue, 21 Jan 2020 09:37:48 +0000
Subject: [PATCH] [IRA] Fix bug 93221 by reverting 11b8091fb

11b8091fb introduced a simplified LRA algorithm for -O0 that turned off
hard register splitting, this causes a problem for parameters passed in
multiple registers on aarch64. This fixes bug 93221.
---
 gcc/ira.c                                  | 38 +++++++++-------------
 gcc/testsuite/gcc.target/aarch64/pr93221.c | 10 ++++++
 2 files changed, 25 insertions(+), 23 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/pr93221.c

diff --git a/gcc/ira.c b/gcc/ira.c
index 46091adf8109263c72343dccfe4913857b5c74ae..c8b5f869da121506f0414901271eae9810689316 100644
--- a/gcc/ira.c
+++ b/gcc/ira.c
@@ -5205,35 +5205,27 @@ ira (FILE *f)
   /* Perform target specific PIC register initialization.  */
   targetm.init_pic_reg ();
 
-  if (optimize)
-    {
-      ira_conflicts_p = true;
-
-      /* Determine the number of pseudos actually requiring coloring.  */
-      unsigned int num_used_regs = 0;
-      for (unsigned int i = FIRST_PSEUDO_REGISTER; i < DF_REG_SIZE (df); i++)
-	if (DF_REG_DEF_COUNT (i) || DF_REG_USE_COUNT (i))
-	  num_used_regs++;
-
-      /* If there are too many pseudos and/or basic blocks (e.g. 10K
-	 pseudos and 10K blocks or 100K pseudos and 1K blocks), we will
-	 use simplified and faster algorithms in LRA.  */
-      lra_simple_p
-	= ira_use_lra_p
-	  && num_used_regs >= (1U << 26) / last_basic_block_for_fn (cfun);
-    }
-  else
-    {
-      ira_conflicts_p = false;
-      lra_simple_p = ira_use_lra_p;
-    }
+  ira_conflicts_p = optimize > 0;
+
+  /* Determine the number of pseudos actually requiring coloring.  */
+  unsigned int num_used_regs = 0;
+  for (unsigned int i = FIRST_PSEUDO_REGISTER; i < DF_REG_SIZE (df); i++)
+    if (DF_REG_DEF_COUNT (i) || DF_REG_USE_COUNT (i))
+      num_used_regs++;
+
+  /* If there are too many pseudos and/or basic blocks (e.g. 10K
+     pseudos and 10K blocks or 100K pseudos and 1K blocks), we will
+     use simplified and faster algorithms in LRA.  */
+  lra_simple_p
+    = ira_use_lra_p
+      && num_used_regs >= (1U << 26) / last_basic_block_for_fn (cfun);
 
   if (lra_simple_p)
     {
       /* It permits to skip live range splitting in LRA.  */
       flag_caller_saves = false;
       /* There is no sense to do regional allocation when we use
-	 simplified LRA.  */
+	simplified LRA.  */
       flag_ira_region = IRA_REGION_ONE;
       ira_conflicts_p = false;
     }
diff --git a/gcc/testsuite/gcc.target/aarch64/pr93221.c b/gcc/testsuite/gcc.target/aarch64/pr93221.c
new file mode 100644
index 0000000000000000000000000000000000000000..4dc2c3d0149423dd3d666f7428277ffa9eb765c4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr93221.c
@@ -0,0 +1,10 @@
+/* PR target/93221 */
+/* { dg-do compile } */
+/* { dg-options "-O0 -mno-omit-leaf-frame-pointer" } */
+
+struct S { __Int32x4_t b[2]; };
+
+void
+foo (struct S x)
+{
+}
-- 
2.17.1
Joel Jan. 21, 2020, 8:18 p.m. UTC | #4
Updated changelog:

Changelog:

2020-01-21  Joel Hutton  <Joel.Hutton@arm.com><mailto:Joel.Hutton@arm.com>

        PR target/93221
        * ira.c (ira): Revert use of simplified LRA algorithm.

gcc/testsuite/ChangeLog:

2020-01-21  Joel Hutton  <Joel.Hutton@arm.com><mailto:Joel.Hutton@arm.com>

        PR target/93221
        * gcc.target/aarch64/pr93221.c: New test.
Joel Jan. 21, 2020, 8:22 p.m. UTC | #5
Changelog was mangled by thunderbird, updated changelog:

Changelog:

2020-01-21  Joel Hutton  <Joel.Hutton@arm.com>
    
         PR target/93221
         * ira.c (ira): Revert use of simplified LRA algorithm.

gcc/testsuite/ChangeLog:

2020-01-21  Joel Hutton  <Joel.Hutton@arm.com>

         PR target/93221
         * gcc.target/aarch64/pr93221.c: New test.
diff mbox series

Patch

From 0d9980d2327c61eb99d041a217d6ea5c5b34c754 Mon Sep 17 00:00:00 2001
From: Joel Hutton <Joel.Hutton@arm.com>
Date: Tue, 21 Jan 2020 09:37:48 +0000
Subject: [PATCH] [IRA] Fix bug 93221 by reverting 11b8091fb

11b8091fb introduced a simplified LRA algorithm for -O0 that turned off
hard register splitting, this causes a problem for parameters passed in
multiple registers on aarch64. This fixes bug 93221.
---
 gcc/ira.c                                  | 38 +++++++++-------------
 gcc/testsuite/gcc.target/aarch64/pr93221.c | 10 ++++++
 2 files changed, 25 insertions(+), 23 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/pr93221.c

diff --git a/gcc/ira.c b/gcc/ira.c
index 46091adf8109263c72343dccfe4913857b5c74ae..c8b5f869da121506f0414901271eae9810689316 100644
--- a/gcc/ira.c
+++ b/gcc/ira.c
@@ -5205,35 +5205,27 @@  ira (FILE *f)
   /* Perform target specific PIC register initialization.  */
   targetm.init_pic_reg ();
 
-  if (optimize)
-    {
-      ira_conflicts_p = true;
-
-      /* Determine the number of pseudos actually requiring coloring.  */
-      unsigned int num_used_regs = 0;
-      for (unsigned int i = FIRST_PSEUDO_REGISTER; i < DF_REG_SIZE (df); i++)
-	if (DF_REG_DEF_COUNT (i) || DF_REG_USE_COUNT (i))
-	  num_used_regs++;
-
-      /* If there are too many pseudos and/or basic blocks (e.g. 10K
-	 pseudos and 10K blocks or 100K pseudos and 1K blocks), we will
-	 use simplified and faster algorithms in LRA.  */
-      lra_simple_p
-	= ira_use_lra_p
-	  && num_used_regs >= (1U << 26) / last_basic_block_for_fn (cfun);
-    }
-  else
-    {
-      ira_conflicts_p = false;
-      lra_simple_p = ira_use_lra_p;
-    }
+  ira_conflicts_p = optimize > 0;
+
+  /* Determine the number of pseudos actually requiring coloring.  */
+  unsigned int num_used_regs = 0;
+  for (unsigned int i = FIRST_PSEUDO_REGISTER; i < DF_REG_SIZE (df); i++)
+    if (DF_REG_DEF_COUNT (i) || DF_REG_USE_COUNT (i))
+      num_used_regs++;
+
+  /* If there are too many pseudos and/or basic blocks (e.g. 10K
+     pseudos and 10K blocks or 100K pseudos and 1K blocks), we will
+     use simplified and faster algorithms in LRA.  */
+  lra_simple_p
+    = ira_use_lra_p
+      && num_used_regs >= (1U << 26) / last_basic_block_for_fn (cfun);
 
   if (lra_simple_p)
     {
       /* It permits to skip live range splitting in LRA.  */
       flag_caller_saves = false;
       /* There is no sense to do regional allocation when we use
-	 simplified LRA.  */
+	simplified LRA.  */
       flag_ira_region = IRA_REGION_ONE;
       ira_conflicts_p = false;
     }
diff --git a/gcc/testsuite/gcc.target/aarch64/pr93221.c b/gcc/testsuite/gcc.target/aarch64/pr93221.c
new file mode 100644
index 0000000000000000000000000000000000000000..517135a889de8a7e379c79222f8a8b2efcc7b422
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr93221.c
@@ -0,0 +1,10 @@ 
+/* PR bug/93221 */
+/* { dg-do compile } */
+/* { dg-options "-O0 -mno-omit-leaf-frame-pointer" } */
+
+struct S { __Int32x4_t b[2]; };
+
+void __attribute__((optimize (0)))
+foo (struct S x)
+{
+}
-- 
2.17.1