Fix copyprop_hardreg_forward_1 (PR rtl-optimization/85342)

Message ID 20180411190343.GC8577@tucnak
State New
Headers show
Series
  • Fix copyprop_hardreg_forward_1 (PR rtl-optimization/85342)
Related show

Commit Message

Jakub Jelinek April 11, 2018, 7:03 p.m.
Hi!

When switching regcprop.c to use validate_* and apply_change_group,
I have added code to restore recog_data.operands[i] if they have been
replaced after apply_change_group failure.  That is bogus though, when
apply_change_group fails, recog_data.insn is NULL and rest of recog_data
structure is complete garbage; and nothing in copyprop_hardreg_forward_1
seems to use it afterwards anyway, just will call extract_insn on the next
insn.  Furthermore, the "fixups" were only for the recog_data structure
operands itself, nothing else, the instruction itself has been already
corrected by cancel_changes.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?

2018-04-11  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/85342
	* regcprop.c (copyprop_hardreg_forward_1): Remove replaced array, use
	a bool scalar var inside of the loop instead.  Don't try to update
	recog_data.operand after failed apply_change_group.

	* gcc.target/i386/pr85342.c: New test.


	Jakub

Comments

Richard Biener April 12, 2018, 8:03 a.m. | #1
On Wed, 11 Apr 2018, Jakub Jelinek wrote:

> Hi!
> 
> When switching regcprop.c to use validate_* and apply_change_group,
> I have added code to restore recog_data.operands[i] if they have been
> replaced after apply_change_group failure.  That is bogus though, when
> apply_change_group fails, recog_data.insn is NULL and rest of recog_data
> structure is complete garbage; and nothing in copyprop_hardreg_forward_1
> seems to use it afterwards anyway, just will call extract_insn on the next
> insn.  Furthermore, the "fixups" were only for the recog_data structure
> operands itself, nothing else, the instruction itself has been already
> corrected by cancel_changes.
> 
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk?

OK.

Richard.

> 2018-04-11  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR rtl-optimization/85342
> 	* regcprop.c (copyprop_hardreg_forward_1): Remove replaced array, use
> 	a bool scalar var inside of the loop instead.  Don't try to update
> 	recog_data.operand after failed apply_change_group.
> 
> 	* gcc.target/i386/pr85342.c: New test.
> 
> --- gcc/regcprop.c.jj	2018-01-04 00:43:17.996703342 +0100
> +++ gcc/regcprop.c	2018-04-11 16:17:29.883575142 +0200
> @@ -751,7 +751,6 @@ copyprop_hardreg_forward_1 (basic_block
>        bool is_asm, any_replacements;
>        rtx set;
>        rtx link;
> -      bool replaced[MAX_RECOG_OPERANDS];
>        bool changed = false;
>        struct kill_set_value_data ksvd;
>  
> @@ -934,7 +933,7 @@ copyprop_hardreg_forward_1 (basic_block
>  	 eldest live copy that's in an appropriate register class.  */
>        for (i = 0; i < n_ops; i++)
>  	{
> -	  replaced[i] = false;
> +	  bool replaced = false;
>  
>  	  /* Don't scan match_operand here, since we've no reg class
>  	     information to pass down.  Any operands that we could
> @@ -951,26 +950,26 @@ copyprop_hardreg_forward_1 (basic_block
>  	  if (recog_data.operand_type[i] == OP_IN)
>  	    {
>  	      if (op_alt[i].is_address)
> -		replaced[i]
> +		replaced
>  		  = replace_oldest_value_addr (recog_data.operand_loc[i],
>  					       alternative_class (op_alt, i),
>  					       VOIDmode, ADDR_SPACE_GENERIC,
>  					       insn, vd);
>  	      else if (REG_P (recog_data.operand[i]))
> -		replaced[i]
> +		replaced
>  		  = replace_oldest_value_reg (recog_data.operand_loc[i],
>  					      alternative_class (op_alt, i),
>  					      insn, vd);
>  	      else if (MEM_P (recog_data.operand[i]))
> -		replaced[i] = replace_oldest_value_mem (recog_data.operand[i],
> -							insn, vd);
> +		replaced = replace_oldest_value_mem (recog_data.operand[i],
> +						     insn, vd);
>  	    }
>  	  else if (MEM_P (recog_data.operand[i]))
> -	    replaced[i] = replace_oldest_value_mem (recog_data.operand[i],
> -						    insn, vd);
> +	    replaced = replace_oldest_value_mem (recog_data.operand[i],
> +						 insn, vd);
>  
>  	  /* If we performed any replacement, update match_dups.  */
> -	  if (replaced[i])
> +	  if (replaced)
>  	    {
>  	      int j;
>  	      rtx new_rtx;
> @@ -989,13 +988,6 @@ copyprop_hardreg_forward_1 (basic_block
>  	{
>  	  if (! apply_change_group ())
>  	    {
> -	      for (i = 0; i < n_ops; i++)
> -		if (replaced[i])
> -		  {
> -		    rtx old = *recog_data.operand_loc[i];
> -		    recog_data.operand[i] = old;
> -		  }
> -
>  	      if (dump_file)
>  		fprintf (dump_file,
>  			 "insn %u: reg replacements not verified\n",
> --- gcc/testsuite/gcc.target/i386/pr85342.c.jj	2018-04-11 16:25:50.564848408 +0200
> +++ gcc/testsuite/gcc.target/i386/pr85342.c	2018-04-11 16:26:05.534856581 +0200
> @@ -0,0 +1,29 @@
> +/* PR rtl-optimization/85342 */
> +/* { dg-do compile { target int128 } } */
> +/* { dg-options "-O2 -mavx512vl" } */
> +
> +typedef unsigned char U __attribute__((vector_size (64)));
> +typedef unsigned int V __attribute__((vector_size (64)));
> +typedef unsigned __int128 W __attribute__((vector_size (64)));
> +int i;
> +V g, h, z, k, l, m;
> +U j;
> +
> +W
> +bar (W o, W p)
> +{
> +  U q;
> +  o |= (W){q[0]} >= o;
> +  o += 1 < o;
> +  j |= (U){} == j;
> +  return i + (W)q + (W)g + (W)h + (W)z + o + (W)j + (W)k + (W)l + (W)m + p;
> +}
> +
> +W
> +foo (U u)
> +{
> +  U q;
> +  W r = bar ((W)(U){0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ~0}, (W)q);
> +  u += (U)bar ((W){~0}, r);
> +  return (W)u;
> +}
> 
> 	Jakub
> 
>

Patch

--- gcc/regcprop.c.jj	2018-01-04 00:43:17.996703342 +0100
+++ gcc/regcprop.c	2018-04-11 16:17:29.883575142 +0200
@@ -751,7 +751,6 @@  copyprop_hardreg_forward_1 (basic_block
       bool is_asm, any_replacements;
       rtx set;
       rtx link;
-      bool replaced[MAX_RECOG_OPERANDS];
       bool changed = false;
       struct kill_set_value_data ksvd;
 
@@ -934,7 +933,7 @@  copyprop_hardreg_forward_1 (basic_block
 	 eldest live copy that's in an appropriate register class.  */
       for (i = 0; i < n_ops; i++)
 	{
-	  replaced[i] = false;
+	  bool replaced = false;
 
 	  /* Don't scan match_operand here, since we've no reg class
 	     information to pass down.  Any operands that we could
@@ -951,26 +950,26 @@  copyprop_hardreg_forward_1 (basic_block
 	  if (recog_data.operand_type[i] == OP_IN)
 	    {
 	      if (op_alt[i].is_address)
-		replaced[i]
+		replaced
 		  = replace_oldest_value_addr (recog_data.operand_loc[i],
 					       alternative_class (op_alt, i),
 					       VOIDmode, ADDR_SPACE_GENERIC,
 					       insn, vd);
 	      else if (REG_P (recog_data.operand[i]))
-		replaced[i]
+		replaced
 		  = replace_oldest_value_reg (recog_data.operand_loc[i],
 					      alternative_class (op_alt, i),
 					      insn, vd);
 	      else if (MEM_P (recog_data.operand[i]))
-		replaced[i] = replace_oldest_value_mem (recog_data.operand[i],
-							insn, vd);
+		replaced = replace_oldest_value_mem (recog_data.operand[i],
+						     insn, vd);
 	    }
 	  else if (MEM_P (recog_data.operand[i]))
-	    replaced[i] = replace_oldest_value_mem (recog_data.operand[i],
-						    insn, vd);
+	    replaced = replace_oldest_value_mem (recog_data.operand[i],
+						 insn, vd);
 
 	  /* If we performed any replacement, update match_dups.  */
-	  if (replaced[i])
+	  if (replaced)
 	    {
 	      int j;
 	      rtx new_rtx;
@@ -989,13 +988,6 @@  copyprop_hardreg_forward_1 (basic_block
 	{
 	  if (! apply_change_group ())
 	    {
-	      for (i = 0; i < n_ops; i++)
-		if (replaced[i])
-		  {
-		    rtx old = *recog_data.operand_loc[i];
-		    recog_data.operand[i] = old;
-		  }
-
 	      if (dump_file)
 		fprintf (dump_file,
 			 "insn %u: reg replacements not verified\n",
--- gcc/testsuite/gcc.target/i386/pr85342.c.jj	2018-04-11 16:25:50.564848408 +0200
+++ gcc/testsuite/gcc.target/i386/pr85342.c	2018-04-11 16:26:05.534856581 +0200
@@ -0,0 +1,29 @@ 
+/* PR rtl-optimization/85342 */
+/* { dg-do compile { target int128 } } */
+/* { dg-options "-O2 -mavx512vl" } */
+
+typedef unsigned char U __attribute__((vector_size (64)));
+typedef unsigned int V __attribute__((vector_size (64)));
+typedef unsigned __int128 W __attribute__((vector_size (64)));
+int i;
+V g, h, z, k, l, m;
+U j;
+
+W
+bar (W o, W p)
+{
+  U q;
+  o |= (W){q[0]} >= o;
+  o += 1 < o;
+  j |= (U){} == j;
+  return i + (W)q + (W)g + (W)h + (W)z + o + (W)j + (W)k + (W)l + (W)m + p;
+}
+
+W
+foo (U u)
+{
+  U q;
+  W r = bar ((W)(U){0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ~0}, (W)q);
+  u += (U)bar ((W){~0}, r);
+  return (W)u;
+}