Patchwork : Fix PR rtl-optimization/50448

login
register
mail settings
Submitter Georg-Johann Lay
Date Nov. 3, 2011, 11:07 a.m.
Message ID <4EB275D8.5070209@gjlay.de>
Download mbox | patch
Permalink /patch/123433/
State New
Headers show

Comments

Georg-Johann Lay - Nov. 3, 2011, 11:07 a.m.
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.
Eric Botcazou - Nov. 3, 2011, 9:21 p.m.
> 	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.
Paolo Bonzini - Nov. 4, 2011, 7:49 a.m.
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
Eric Botcazou - Nov. 4, 2011, 8:50 a.m.
> 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".
Paolo Bonzini - Nov. 4, 2011, 9:41 a.m.
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
Eric Botcazou - Nov. 4, 2011, 9:44 a.m.
> 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.

Patch

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