Patchwork Fix up ARM ICE (PR target/49069)

login
register
mail settings
Submitter Jakub Jelinek
Date Jan. 21, 2013, 10:55 a.m.
Message ID <20130121105515.GH7269@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/214080/
State New
Headers show

Comments

Jakub Jelinek - Jan. 21, 2013, 10:55 a.m.
Hi!

As can be seen on the testcase, this backend bug is still reproduceable even
on trunk, the backend just can't rely on cstoredi4 or cbranchdi4 expansion
not being performed with two constants, unless it has predicates that
disallow it (Steven's patch in the PR).
This patch just forces it into registers instead, it will be simplified by
RTL optimizers anyway later on (or another alternative is FAIL there in that
case, see my other patch in the PR).
I really don't care which way this is fixed, but having such ICE around for
so long when the fix is so easy is unnecessary.
Tested just on the testcase, given that previously we'd always ICE on it,
it can't make things worse.

Ok for trunk?

2013-01-21  Jakub Jelinek  <jakub@redhat.com>

	PR target/49069
	* config/arm/arm.md (cbranchdi4, cstoredi4): Don't ICE if
	both comparison operands are constants.

	* gcc.dg/pr49069.c: New test.


	Jakub
Ramana Radhakrishnan - Jan. 22, 2013, 10:38 a.m.
On 01/21/13 10:55, Jakub Jelinek wrote:
> Hi!
>
> As can be seen on the testcase, this backend bug is still reproduceable even
> on trunk, the backend just can't rely on cstoredi4 or cbranchdi4 expansion
> not being performed with two constants, unless it has predicates that
> disallow it (Steven's patch in the PR).
> This patch just forces it into registers instead, it will be simplified by
> RTL optimizers anyway later on (or another alternative is FAIL there in that
> case, see my other patch in the PR).
> I really don't care which way this is fixed, but having such ICE around for
> so long when the fix is so easy is unnecessary.
> Tested just on the testcase, given that previously we'd always ICE on it,
> it can't make things worse.
>
> Ok for trunk?
>
> 2013-01-21  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR target/49069
> 	* config/arm/arm.md (cbranchdi4, cstoredi4): Don't ICE if
> 	both comparison operands are constants.
>
> 	* gcc.dg/pr49069.c: New test.
>
> --- gcc/config/arm/arm.md.jj	2013-01-11 09:03:13.000000000 +0100
> +++ gcc/config/arm/arm.md	2013-01-17 16:57:58.246233079 +0100
> @@ -7035,9 +7035,10 @@ (define_expand "cbranchdi4"
>   	      (pc)))]
>     "TARGET_32BIT"
>     "{
> -     /* We should not have two constants.  */
> -     gcc_assert (GET_MODE (operands[1]) == DImode
> -		 || GET_MODE (operands[2]) == DImode);
> +     /* If we have two constants, force one into register.  */
> +     if (GET_MODE (operands[1]) != DImode
> +	 && GET_MODE (operands[2]) != DImode)
> +	operands[1] = force_reg (DImode, operands[1]);

I don't know where we've got to with respect to providing CONST_INTs 
modes these days but given that's on the cards I'd rather not have such 
mechanisms for detecting both operands being const_ints here .

Instead I'd just use s_register_operand for operand1 and continue to use 
cmpdi_operand for operand2 and fix it so.

>
>        if (!arm_validize_comparison (&operands[0], &operands[1], &operands[2]))
>          FAIL;
> @@ -7958,9 +7959,10 @@ (define_expand "cstoredi4"
>   	  (match_operand:DI 3 "cmpdi_operand" "")]))]
>     "TARGET_32BIT"
>     "{
> -     /* We should not have two constants.  */
> -     gcc_assert (GET_MODE (operands[2]) == DImode
> -		 || GET_MODE (operands[3]) == DImode);
> +     /* If we have two constants, force one into register.  */
> +     if (GET_MODE (operands[2]) != DImode
> +	 && GET_MODE (operands[3]) != DImode)
> +	operands[2] = force_reg (DImode, operands[2]);

And likewise .

Ok with those changes and if no regressions.

regards
Ramana

Patch

--- gcc/config/arm/arm.md.jj	2013-01-11 09:03:13.000000000 +0100
+++ gcc/config/arm/arm.md	2013-01-17 16:57:58.246233079 +0100
@@ -7035,9 +7035,10 @@  (define_expand "cbranchdi4"
 	      (pc)))]
   "TARGET_32BIT"
   "{
-     /* We should not have two constants.  */
-     gcc_assert (GET_MODE (operands[1]) == DImode
-		 || GET_MODE (operands[2]) == DImode);
+     /* If we have two constants, force one into register.  */
+     if (GET_MODE (operands[1]) != DImode
+	 && GET_MODE (operands[2]) != DImode)
+	operands[1] = force_reg (DImode, operands[1]);
 
      if (!arm_validize_comparison (&operands[0], &operands[1], &operands[2]))
        FAIL;
@@ -7958,9 +7959,10 @@  (define_expand "cstoredi4"
 	  (match_operand:DI 3 "cmpdi_operand" "")]))]
   "TARGET_32BIT"
   "{
-     /* We should not have two constants.  */
-     gcc_assert (GET_MODE (operands[2]) == DImode
-		 || GET_MODE (operands[3]) == DImode);
+     /* If we have two constants, force one into register.  */
+     if (GET_MODE (operands[2]) != DImode
+	 && GET_MODE (operands[3]) != DImode)
+	operands[2] = force_reg (DImode, operands[2]);
 
      if (!arm_validize_comparison (&operands[1],
      				   &operands[2],
--- gcc/testsuite/gcc.dg/pr49069.c.jj	2012-11-17 15:43:17.572007394 +0100
+++ gcc/testsuite/gcc.dg/pr49069.c	2013-01-17 16:43:41.613146835 +0100
@@ -0,0 +1,15 @@ 
+/* PR target/49069 */
+/* { dg-do compile } */
+/* { dg-options "-Os -fno-tree-forwprop -Wno-div-by-zero" } */
+
+int a;
+const unsigned long long b[1] = { 1ULL };
+extern void bar (int);
+
+void
+foo (void)
+{
+  for (a = 0; a == 1; a = 2)
+    ;
+  bar (b[0] == (a == 0 ? a : a / 0));
+}