Patchwork PATCH: Remove redundant instructions after regcprop

login
register
mail settings
Submitter Paulo J. Matos
Date Aug. 2, 2012, 10:17 a.m.
Message ID <jvdk3m$gt7$1@dough.gmane.org>
Download mbox | patch
Permalink /patch/174714/
State New
Headers show

Comments

Paulo J. Matos - Aug. 2, 2012, 10:17 a.m.
Extended regcprop to check and remove for redundant move instructions 
resulting from the pass.

Paulo.

2012-08-02   Paulo Matos <Paulo.Matos@csr.com>

	* regcprop.c (copy_value): remove check for redundant moves.
	* regcprop.c (copy_value): add check for redundant moves,
	remove instructions if redundant.
Paulo J. Matos - Aug. 2, 2012, 10:19 a.m.
Forgot to mention: this is to fix PR 54154.

Updated changelog:

2012-08-02   Paulo Matos <Paulo.Matos@csr.com>

     PR middle-end/54154
     * regcprop.c (copy_value): remove check for redundant moves.
     * regcprop.c (copy_value): add check for redundant moves,
     remove instructions if redundant.
Steven Bosscher - Aug. 2, 2012, 10:25 a.m.
On Thu, Aug 2, 2012 at 12:17 PM, Paulo J. Matos <paulo at matos-sorge
dot com> wrote:
> Extended regcprop to check and remove for redundant move instructions
> resulting from the pass.
>
> Paulo.
>
> 2012-08-02   Paulo Matos <Paulo dot Matos at csr dot com>
>
>         * regcprop.c (copy_value): remove check for redundant moves.
>         * regcprop.c (copy_value): add check for redundant moves,
>         remove instructions if redundant.
>

Hello,

Thanks for working on this.

How did you test this? How do you account for mode differences between
the two registers?

Please also fix the patch (and the ChangeLog entry) to conform to the
GCC coding style requirements.

Ciao!
Steven
Richard Guenther - Aug. 2, 2012, 10:27 a.m.
On Thu, Aug 2, 2012 at 12:19 PM, Paulo J. Matos <paulo@matos-sorge.com> wrote:
> Forgot to mention: this is to fix PR 54154.
>
> Updated changelog:
>
> 2012-08-02   Paulo Matos <Paulo.Matos@csr.com>
>
>     PR middle-end/54154
>
>     * regcprop.c (copy_value): remove check for redundant moves.
>     * regcprop.c (copy_value): add check for redundant moves,
>     remove instructions if redundant.

That's in copyprop_hardreg_forward_1

+  gcc_assert(dr != sr);
+

space before ().

+  FOR_BB_INSNS_SAFE(bb, insn, next)
     {

Likewise.

+        unsigned int dr = REGNO(SET_DEST(set));
+        unsigned int sr = REGNO(SET_SRC(set));
+

Likewise.

Richard.

>
>
>
Paulo J. Matos - Aug. 2, 2012, 1:22 p.m.
Thanks for the comments, I will be sending a new patch and fixed changelog.

On 02/08/12 11:27, Richard Guenther wrote:
> On Thu, Aug 2, 2012 at 12:19 PM, Paulo J. Matos <paulo@matos-sorge.com> wrote:
>> Forgot to mention: this is to fix PR 54154.
>>
>> Updated changelog:
>>
>> 2012-08-02   Paulo Matos <Paulo.Matos@csr.com>
>>
>>      PR middle-end/54154
>>
>>      * regcprop.c (copy_value): remove check for redundant moves.
>>      * regcprop.c (copy_value): add check for redundant moves,
>>      remove instructions if redundant.
>
> That's in copyprop_hardreg_forward_1
>
> +  gcc_assert(dr != sr);
> +
>
> space before ().
>
> +  FOR_BB_INSNS_SAFE(bb, insn, next)
>       {
>
> Likewise.
>
> +        unsigned int dr = REGNO(SET_DEST(set));
> +        unsigned int sr = REGNO(SET_SRC(set));
> +
>
> Likewise.
>
> Richard.
>
>>
>>
>>
>
Paulo J. Matos - Aug. 2, 2012, 1:42 p.m.
On 02/08/12 11:25, Steven Bosscher wrote:
> Hello,
>
> Thanks for working on this.
>
> How did you test this?
>

This patch is for GCC46. The main problem is that I was not able to 
reproduce it (yet) for any upstream backends. I therefore patched GCC46 
and tested our backend (where I can easily reproduce the problem) with 
our commercial test suites and our own hand-crafted suite.

I would be eager to know if you have any suggestion on how to test this 
with upstream backends even if we can't reproduce it. One way would be 
to test trunk before and after patch and see if we introduce any 
testsuite regressions, but it feels slightly useless if this problem 
doesn't occur with x86 (maybe due to the large amount of registers, or 
how move rules are described in the backend).

 > How do you account for mode differences between
 > the two registers?

 From what I understand about the code is that a simple move between two 
registers will always have registers with the same mode, so mode is not 
a worry. That was also the assumption of the code that was written 
before which had in copy_value:
/* Assert that SRC has been copied to DEST.  Adjust the data structures to
reflect that SRC contains an older copy of the shared value.  */

static void
copy_value (rtx dest, rtx src, struct value_data *vd)
{
   unsigned int dr = REGNO (dest);
   unsigned int sr = REGNO (src);
   unsigned int dn, sn;
   unsigned int i;

   /* ??? At present, it's possible to see noop sets.  It'd be nice if 
this were
cleaned up beforehand...  */
   if (sr == dr)
     return;

> Please also fix the patch (and the ChangeLog entry) to conform to the
> GCC coding style requirements.
>

Richard already sent me a message about that.
Those problems were lack of experience with patch submission. Will sort 
that out and resend.

Thanks for your interest in the patch,

Paulo Matos

> Ciao!
> Steven
>

Patch

--- //depot/bc/main/devHost/gcc46/gcc/gcc/regcprop.c	2011-09-06 14:29:15.000000000 0100
+++ /home/pm18/p4ws/pm18_binutils/bc/main/devHost/gcc46/gcc/gcc/regcprop.c	2011-09-06 14:29:15.000000000 0100
@@ -301,11 +301,8 @@ 
   unsigned int dn, sn;
   unsigned int i;
 
-  /* ??? At present, it's possible to see noop sets.  It'd be nice if
-     this were cleaned up beforehand...  */
-  if (sr == dr)
-    return;
-
+  gcc_assert(dr != sr);
+  
   /* Do not propagate copies to the stack pointer, as that can leave
      memory accesses with no scheduling dependency on the stack update.  */
   if (dr == STACK_POINTER_REGNUM)
@@ -734,9 +731,9 @@ 
 copyprop_hardreg_forward_1 (basic_block bb, struct value_data *vd)
 {
   bool anything_changed = false;
-  rtx insn;
+  rtx insn, next;
 
-  for (insn = BB_HEAD (bb); ; insn = NEXT_INSN (insn))
+  FOR_BB_INSNS_SAFE(bb, insn, next)
     {
       int n_ops, i, alt, predicated;
       bool is_asm, any_replacements;
@@ -755,10 +752,7 @@ 
 					   insn, vd);
 	    }
 
-	  if (insn == BB_END (bb))
-	    break;
-	  else
-	    continue;
+	  continue;
 	}
 
       set = single_set (insn);
@@ -966,10 +960,19 @@ 
 
       /* Notice copies.  */
       if (set && REG_P (SET_DEST (set)) && REG_P (SET_SRC (set)))
-	copy_value (SET_DEST (set), SET_SRC (set), vd);
-
-      if (insn == BB_END (bb))
-	break;
+      {
+        unsigned int dr = REGNO(SET_DEST(set));
+        unsigned int sr = REGNO(SET_SRC(set));
+        
+        if(dr == sr)
+        {
+          /* noop set */
+          delete_insn_and_edges(insn);
+        } 
+        else
+          copy_value (SET_DEST (set), SET_SRC (set), vd);
+      }
+      
     }
 
   return anything_changed;