[PATCH, rtl-optimization]: Fix PR60851, ICE: in extract_constrain_insn_cached, at recog.c

[PATCH, rtl-optimization]: Fix PR60851, ICE: in extract_constrain_insn_cached, at recog.c

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

Hello!

Attached patch activates "safety net" recognition of pseudo regs for
LRA-enabled targets. LRA is able to spill pseudos to memory to satisfy
memory-only constraints, so constrain_operands should recognize this
functionality.

This is actually the same approach as how constants are handled a
couple of lines above. the changed section.

2015-03-18  Uros Bizjak  <ubizjak@gmail.com>

    PR rtl-optimization/60851
    * recog.c (constrain_operands): Accept a pseudo register before reload
    for LRA enabled targets.

testsuite/ChangeLog:

2015-03-18  Uros Bizjak  <ubizjak@gmail.com>

    PR rtl-optimization/60851
    * gcc.target/i386/pr60851.c: New test.

Patch was bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.

OK for mainline and 4.9 branch?

Uros.

[-- Attachment #2: p.diff.txt --]
[-- Type: text/plain, Size: 1074 bytes --]

Index: recog.c
===================================================================
--- recog.c	(revision 221493)
+++ recog.c	(working copy)
@@ -2775,6 +2775,10 @@ constrain_operands (int strict, alternative_mask a
 			       /* Before reload, accept what reload can turn
 				  into mem.  */
 			       || (strict < 0 && CONSTANT_P (op))
+			       /* Before reload, accept a pseudo,
+				  since LRA can turn it into mem.  */
+			       || (targetm.lra_p () && strict < 0 && REG_P (op)
+				   && REGNO (op) >= FIRST_PSEUDO_REGISTER)
 			       /* During reload, accept a pseudo  */
 			       || (reload_in_progress && REG_P (op)
 				   && REGNO (op) >= FIRST_PSEUDO_REGISTER)))
Index: testsuite/gcc.target/i386/pr60851.c
===================================================================
--- testsuite/gcc.target/i386/pr60851.c	(revision 0)
+++ testsuite/gcc.target/i386/pr60851.c	(working copy)
@@ -0,0 +1,7 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -flive-range-shrinkage -mtune=bdver4 -mdispatch-scheduler" } */
+
+long double ld (char c)
+{
+  return c;
+}

Re: [PATCH, rtl-optimization]: Fix PR60851, ICE: in extract_constrain_insn_cached, at recog.c

[Jakub Jelinek]

On Wed, Mar 18, 2015 at 08:00:33PM +0100, Uros Bizjak wrote:
> 2015-03-18  Uros Bizjak  <ubizjak@gmail.com>
> 
>     PR rtl-optimization/60851
>     * recog.c (constrain_operands): Accept a pseudo register before reload
>     for LRA enabled targets.
> 
> --- recog.c	(revision 221493)
> +++ recog.c	(working copy)
> @@ -2775,6 +2775,10 @@ constrain_operands (int strict, alternative_mask a
>  			       /* Before reload, accept what reload can turn
>  				  into mem.  */
>  			       || (strict < 0 && CONSTANT_P (op))
> +			       /* Before reload, accept a pseudo,
> +				  since LRA can turn it into mem.  */
> +			       || (targetm.lra_p () && strict < 0 && REG_P (op)
> +				   && REGNO (op) >= FIRST_PSEUDO_REGISTER)
>  			       /* During reload, accept a pseudo  */
>  			       || (reload_in_progress && REG_P (op)
>  				   && REGNO (op) >= FIRST_PSEUDO_REGISTER)))

This looks reasonable to me, but please give Vlad an extra day to comment on
it.

As the two adjacent conditions are mostly the same, perhaps it might be
better written as
			       || ((/* Before reload, accept a pseudo, since
				       LRA can turn it into a mem.
				    (targetm.lra_p () && strict < 0)
				    /* During reload, accept a pseudo.  */
				    || reload_in_progress)
				   && REG_P (op)
				   && REGNO (op) >= FIRST_PSEUDO_REGISTER)))

or put REG_P && REGNO checks first and only then test when.

For 4.9 backport, please wait a few days after it goes into the trunk.

	Jakub

Re: [PATCH, rtl-optimization]: Fix PR60851, ICE: in extract_constrain_insn_cached, at recog.c

[Uros Bizjak]

On Thu, Mar 19, 2015 at 8:44 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Mar 18, 2015 at 08:00:33PM +0100, Uros Bizjak wrote:
>> 2015-03-18  Uros Bizjak  <ubizjak@gmail.com>
>>
>>     PR rtl-optimization/60851
>>     * recog.c (constrain_operands): Accept a pseudo register before reload
>>     for LRA enabled targets.

> As the two adjacent conditions are mostly the same, perhaps it might be
> better written as
>                                || ((/* Before reload, accept a pseudo, since
>                                        LRA can turn it into a mem.
>                                     (targetm.lra_p () && strict < 0)
>                                     /* During reload, accept a pseudo.  */
>                                     || reload_in_progress)
>                                    && REG_P (op)
>                                    && REGNO (op) >= FIRST_PSEUDO_REGISTER)))
>
> or put REG_P && REGNO checks first and only then test when.

Yeah, I thought about this particular CSE a bit. But since these are
two conceptually different conditions, and the formatting (and
comments) just didn't fit into available space, I wrote it this way.
IMO, it is more readable, better follows the intended logic, and
avoids even more indents.

Also, I am pretty sure that any decent compiler can CSE this part on its own.

However, the condition can be slightly improved by rewriting it to:

                   /* Before reload, accept a pseudo,
                  since LRA can turn it into a mem.  */
                   || (strict < 0 && targetm.lra_p () && REG_P (op)
                   && REGNO (op) >= FIRST_PSEUDO_REGISTER)

so, we have cheaper tests in the front to shortcut more expensive tests later.

> For 4.9 backport, please wait a few days after it goes into the trunk.

Thanks!

Uros.

Re: [PATCH, rtl-optimization]: Fix PR60851, ICE: in extract_constrain_insn_cached, at recog.c

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 2368 bytes --]

On Thu, Mar 19, 2015 at 9:38 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Thu, Mar 19, 2015 at 8:44 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Wed, Mar 18, 2015 at 08:00:33PM +0100, Uros Bizjak wrote:
>>> 2015-03-18  Uros Bizjak  <ubizjak@gmail.com>
>>>
>>>     PR rtl-optimization/60851
>>>     * recog.c (constrain_operands): Accept a pseudo register before reload
>>>     for LRA enabled targets.
>
>> As the two adjacent conditions are mostly the same, perhaps it might be
>> better written as
>>                                || ((/* Before reload, accept a pseudo, since
>>                                        LRA can turn it into a mem.
>>                                     (targetm.lra_p () && strict < 0)
>>                                     /* During reload, accept a pseudo.  */
>>                                     || reload_in_progress)
>>                                    && REG_P (op)
>>                                    && REGNO (op) >= FIRST_PSEUDO_REGISTER)))
>>
>> or put REG_P && REGNO checks first and only then test when.
>
> Yeah, I thought about this particular CSE a bit. But since these are
> two conceptually different conditions, and the formatting (and
> comments) just didn't fit into available space, I wrote it this way.
> IMO, it is more readable, better follows the intended logic, and
> avoids even more indents.
>
> Also, I am pretty sure that any decent compiler can CSE this part on its own.
>
> However, the condition can be slightly improved by rewriting it to:
>
>                    /* Before reload, accept a pseudo,
>                   since LRA can turn it into a mem.  */
>                    || (strict < 0 && targetm.lra_p () && REG_P (op)
>                    && REGNO (op) >= FIRST_PSEUDO_REGISTER)
>
> so, we have cheaper tests in the front to shortcut more expensive tests later.
>
>> For 4.9 backport, please wait a few days after it goes into the trunk.

Attached is the 4.9 branch version of the patch. There were
substantial cleanups [1] in this area for 5.0, so comparing to trunk
version, the patch includes two more places that have to be changed.

Patch was bootstrapped and regression tested on x86_64-linux-gnu
{,-m32}. If there are no objections, I plan to commit this version to
the branch in a couple of days.

[1] https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00530.html

Uros.

[-- Attachment #2: p-49.diff.txt --]
[-- Type: text/plain, Size: 2284 bytes --]

Index: testsuite/gcc.target/i386/pr60851.c
===================================================================
--- testsuite/gcc.target/i386/pr60851.c	(revision 0)
+++ testsuite/gcc.target/i386/pr60851.c	(revision 0)
@@ -0,0 +1,7 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -flive-range-shrinkage -mtune=bdver4 -mdispatch-scheduler" } */
+
+long double ld (char c)
+{
+  return c;
+}
Index: recog.c
===================================================================
--- recog.c	(revision 221587)
+++ recog.c	(working copy)
@@ -2627,9 +2627,14 @@ constrain_operands (int strict)
 		      break;
 		    win = 1;
 		  }
-		/* Before reload, accept what reload can turn into mem.  */
+		/* Before reload, accept what reload can turn into a mem.  */
 		else if (strict < 0 && CONSTANT_P (op))
 		  win = 1;
+		/* Before reload, accept a pseudo,
+		   since LRA can turn it into a mem.  */
+		else if (strict < 0 && targetm.lra_p () && REG_P (op)
+			 && REGNO (op) >= FIRST_PSEUDO_REGISTER)
+		  win = 1;
 		/* During reload, accept a pseudo  */
 		else if (reload_in_progress && REG_P (op)
 			 && REGNO (op) >= FIRST_PSEUDO_REGISTER)
@@ -2708,6 +2713,10 @@ constrain_operands (int strict)
 		    /* Before reload, accept what reload can handle.  */
 		    || (strict < 0
 			&& (CONSTANT_P (op) || MEM_P (op)))
+		    /* Before reload, accept a pseudo,
+		       since LRA can turn it into a mem.  */
+		    || (strict < 0 && targetm.lra_p () && REG_P (op)
+			&& REGNO (op) >= FIRST_PSEUDO_REGISTER)
 		    /* During reload, accept a pseudo  */
 		    || (reload_in_progress && REG_P (op)
 			&& REGNO (op) >= FIRST_PSEUDO_REGISTER))
@@ -2739,8 +2748,12 @@ constrain_operands (int strict)
 			   /* Every memory operand can be reloaded to fit.  */
 			   && ((strict < 0 && MEM_P (op))
 			       /* Before reload, accept what reload can turn
-				  into mem.  */
+				  into a mem.  */
 			       || (strict < 0 && CONSTANT_P (op))
+			       /* Before reload, accept a pseudo,
+				  since LRA can turn it into a mem.  */
+			       || (strict < 0 && targetm.lra_p () && REG_P (op)
+				   && REGNO (op) >= FIRST_PSEUDO_REGISTER)
 			       /* During reload, accept a pseudo  */
 			       || (reload_in_progress && REG_P (op)
 				   && REGNO (op) >= FIRST_PSEUDO_REGISTER)))