[Patch]: Fix PR rtl-optimization/50448

[Patch]: Fix PR rtl-optimization/50448

[Georg-Johann Lay]

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

This is a patch as proposed by paolo to fix a missing propagaton of const-int
addresses.

Bootstrapped and regression tested on i686-pc-linux-gnu.

Ok for trunk?

Johann

	PR rtl-optimization/50448
	* cprop.c (try_replace_reg): Try to simplify SET_SRC given the
	substitution.

[-- Attachment #2: cprop-set-dest.diff --]
[-- Type: text/x-patch, Size: 942 bytes --]

Index: cprop.c
===================================================================
--- cprop.c	(revision 180654)
+++ cprop.c	(working copy)
@@ -764,6 +764,18 @@ try_replace_reg (rtx from, rtx to, rtx i
 	note = set_unique_reg_note (insn, REG_EQUAL, copy_rtx (src));
     }
 
+  if (set && MEM_P (SET_DEST (set)) && reg_mentioned_p (from, SET_DEST (set)))
+    {
+      /* If above failed and this is a single set, try to simplify the source of
+	 the set given our substitution.  We could perhaps try this for multiple
+	 SETs, but it probably won't buy us anything.  */
+      rtx addr = simplify_replace_rtx (SET_DEST (set), from, to);
+
+      if (!rtx_equal_p (addr, SET_DEST (set))
+	  && validate_change (insn, &SET_DEST (set), addr, 0))
+	success = 1;
+    }
+
   /* REG_EQUAL may get simplified into register.
      We don't allow that. Remove that note. This code ought
      not to happen, because previous code ought to synthesize

Re: [Patch]: Fix PR rtl-optimization/50448

[Eric Botcazou]

> 	PR rtl-optimization/50448
> 	* cprop.c (try_replace_reg): Try to simplify SET_SRC given the
> 	substitution.

The whole patch is about SET_DEST though, so I'm a little confused.  And the 
head comment of try_replace_reg reads:

/* Try to replace all non-SET_DEST occurrences of FROM in INSN with TO.
   Returns nonzero is successful.  */

so I'm not sure this is the right place to fiddle with SET_DEST.

-- 
Eric Botcazou

Re: [Patch]: Fix PR rtl-optimization/50448

[Paolo Bonzini]

On 11/03/2011 10:21 PM, Eric Botcazou wrote:
>> 	PR rtl-optimization/50448
>> >  	* cprop.c (try_replace_reg): Try to simplify SET_SRC given the
>> >  	substitution.
> The whole patch is about SET_DEST though, so I'm a little confused.

Yes, the changelog is wrong indeed.  Registers in a SET_DEST memory are 
uses, so they are like SET_SRC in this context which is why I think the 
patch does belong in try_replace_reg.  Georg, what do you think of a 
changelog like this:

   Also try to replace uses of FROM that appear in SET_DEST.

> And the
> head comment of try_replace_reg reads:
>
> /* Try to replace all non-SET_DEST occurrences of FROM in INSN with TO.
>     Returns nonzero is successful.  */

I agree; like above, the patch should also change the head comment like 
this:

/* Try to replace all uses of FROM in INSN with TO.  Returns
    nonzero is successful.  */

Paolo

Re: [Patch]: Fix PR rtl-optimization/50448

[Eric Botcazou]

> Yes, the changelog is wrong indeed.  Registers in a SET_DEST memory are
> uses, so they are like SET_SRC in this context which is why I think the
> patch does belong in try_replace_reg.  Georg, what do you think of a
> changelog like this:
>
>    Also try to replace uses of FROM that appear in SET_DEST.

OK, this makes sense.  validate_replace_src_group actually does this too.

> I agree; like above, the patch should also change the head comment like
> this:
>
> /* Try to replace all uses of FROM in INSN with TO.  Returns
>     nonzero is successful.  */

No 's' in "Returns".  Note that the comment is also off:

+   /* If above failed and this is a single set, try to simplify the source of
+      the set given our substitution.  We could perhaps try this for multiple
+      SETs, but it probably won't buy us anything.  */
+    rtx addr = simplify_replace_rtx (SET_DEST (set), from, to);

What does "If above failed" refer to?  Again "source" instead of "destination".

-- 
Eric Botcazou

Re: [Patch]: Fix PR rtl-optimization/50448

[Paolo Bonzini]

On 11/04/2011 09:50 AM, Eric Botcazou wrote:
> +   /* If above failed and this is a single set, try to simplify the source of
> +      the set given our substitution.  We could perhaps try this for multiple
> +      SETs, but it probably won't buy us anything.  */
> +    rtx addr = simplify_replace_rtx (SET_DEST (set), from, to);
>
> What does "If above failed" refer to?  Again "source" instead of "destination".

What about

    /* Registers can also appear as uses in SET_DEST if it is a MEM.  We
       could perhaps try this for multiple SETs, but it probably won't
       buy us anything.  */

?

Georg, can you put it all together into a v2?

Paolo

Re: [Patch]: Fix PR rtl-optimization/50448

[Eric Botcazou]

> What about
>
>     /* Registers can also appear as uses in SET_DEST if it is a MEM.  We
>        could perhaps try this for multiple SETs, but it probably won't
>        buy us anything.  */
>
> ?

Fine with me, thanks.

-- 
Eric Botcazou

Re: [Patch]: Fix PR rtl-optimization/50448

[Georg-Johann Lay]

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

Paolo Bonzini wrote:
> On 11/04/2011 09:50 AM, Eric Botcazou wrote:
>> +   /* If above failed and this is a single set, try to simplify the
>> source of
>> +      the set given our substitution.  We could perhaps try this for
>> multiple
>> +      SETs, but it probably won't buy us anything.  */
>> +    rtx addr = simplify_replace_rtx (SET_DEST (set), from, to);
>>
>> What does "If above failed" refer to?  Again "source" instead of
>> "destination".
> 
> What about
> 
>    /* Registers can also appear as uses in SET_DEST if it is a MEM.  We
>       could perhaps try this for multiple SETs, but it probably won't
>       buy us anything.  */
> 
> ?
> 
> Georg, can you put it all together into a v2?
> 
> Paolo

Like so?

	PR rtl-optimization/50448
	* cprop.c (try_replace_reg): Also try to replace uses of FROM that
	appear in SET_DEST.

IMO the head comment is still misleading because it might give rise to the
assumption that SET_DESTs are replaced, too, which is not the case.

Johann



[-- Attachment #2: cprop-set-dest.diff --]
[-- Type: text/x-patch, Size: 1269 bytes --]

Index: cprop.c
===================================================================
--- cprop.c	(revision 180962)
+++ cprop.c	(working copy)
@@ -712,8 +712,8 @@ find_used_regs (rtx *xptr, void *data AT
     }
 }
 
-/* Try to replace all non-SET_DEST occurrences of FROM in INSN with TO.
-   Returns nonzero is successful.  */
+/* Try to replace all uses of FROM in INSN with TO.
+   Return nonzero if successful.  */
 
 static int
 try_replace_reg (rtx from, rtx to, rtx insn)
@@ -764,6 +764,18 @@ try_replace_reg (rtx from, rtx to, rtx i
 	note = set_unique_reg_note (insn, REG_EQUAL, copy_rtx (src));
     }
 
+  if (set && MEM_P (SET_DEST (set)) && reg_mentioned_p (from, SET_DEST (set)))
+    {
+      /* Registers can also appear as uses in SET_DEST if it is a MEM.
+         We could perhaps try this for multiple SETs, but it probably
+         won't buy us anything.  */
+      rtx addr = simplify_replace_rtx (SET_DEST (set), from, to);
+      
+      if (!rtx_equal_p (addr, SET_DEST (set))
+          && validate_change (insn, &SET_DEST (set), addr, 0))
+        success = 1;
+    }
+
   /* REG_EQUAL may get simplified into register.
      We don't allow that. Remove that note. This code ought
      not to happen, because previous code ought to synthesize

Re: [Patch]: Fix PR rtl-optimization/50448

[Eric Botcazou]

> 	PR rtl-optimization/50448
> 	* cprop.c (try_replace_reg): Also try to replace uses of FROM that
> 	appear in SET_DEST.

OK if it passes testing, with s/addr/dest/ as addr isn't an address at all.

> IMO the head comment is still misleading because it might give rise to the
> assumption that SET_DESTs are replaced, too, which is not the case.

"use" is to be understood as opposed to "set" so SET_DEST itself is excluded.

-- 
Eric Botcazou

Re: [Patch]: Fix PR rtl-optimization/50448

[Georg-Johann Lay]

Eric Botcazou wrote:
>> 	PR rtl-optimization/50448
>> 	* cprop.c (try_replace_reg): Also try to replace uses of FROM that
>> 	appear in SET_DEST.
> 
> OK if it passes testing, with s/addr/dest/ as addr isn't an address at all.

Ok, it's here:

http://gcc.gnu.org/viewcvs?view=revision&revision=181011

Johann