Patchwork Fix combiner (PRs rtl-optimization/46034, rtl-optimization/46212, rtl-optimization/46248)

login
register
mail settings
Submitter Eric Botcazou
Date Nov. 2, 2010, 8:27 p.m.
Message ID <201011022127.29192.ebotcazou@adacore.com>
Download mbox | patch
Permalink /patch/69923/
State New
Headers show

Comments

Eric Botcazou - Nov. 2, 2010, 8:27 p.m.
> There are two issues, one is that the earlier
> newpat = subst (newpat, i0dest, i0src, ...);
> might have (but not necessarily) have changed i1src and so when i1dest
> is first replaced with i1src that way modified and then i0dest is replaced
> with i0src, the replacements are already wrong and as testcases show
> self-referential.

FWIW I also debugged this (and spotted a pasto in the 4-insn combiner patch 
that I'll fix after your fixes, patch attached).  I also roughly came up with:

+      /* Following subst may modify i1src, make a copy of it
+        before it is for added_sets_2 handling if needed.  */
+      if (added_sets_2
+         && i0dest_in_i0src
+         && i0_feeds_i1_n
+         && (i1_feeds_i2_n || i0_feeds_i2_n))
+       i1src_copy = copy_rtx (i1src);

but why not just

 if (i0_feeds_i1_n && added_sets_2 && i1_feeds_i2_n)

i.e. make a copy if substituting I0 will clobber I1SRC and I1SRC will be re- 
substituted in I2PAT?

> The other issue is that if we are to apply more than 
> one substitution on i2pat and i0dest_in_i0src, then we need to pass
> 1 as last argument to the first subst in order to avoid unwanted
> rtl sharing (which again can lead to self-referential rtl).
> Another issue is that if all of i0_feeds_i2_n, i0_feeds_i1_n and
> i1_feeds_i2_n is true, then we'd be substituting i0dest with i0src
> in i2pat twice.

This part looks OK to me.


	* combine.c (try_combine): Fix formatting issues and a pasto.
Jakub Jelinek - Nov. 2, 2010, 8:48 p.m.
On Tue, Nov 02, 2010 at 09:27:29PM +0100, Eric Botcazou wrote:
> > There are two issues, one is that the earlier
> > newpat = subst (newpat, i0dest, i0src, ...);
> > might have (but not necessarily) have changed i1src and so when i1dest
> > is first replaced with i1src that way modified and then i0dest is replaced
> > with i0src, the replacements are already wrong and as testcases show
> > self-referential.
> 
> FWIW I also debugged this (and spotted a pasto in the 4-insn combiner patch 
> that I'll fix after your fixes, patch attached).  I also roughly came up with:
> 
> +      /* Following subst may modify i1src, make a copy of it
> +        before it is for added_sets_2 handling if needed.  */
> +      if (added_sets_2
> +         && i0dest_in_i0src
> +         && i0_feeds_i1_n
> +         && (i1_feeds_i2_n || i0_feeds_i2_n))
> +       i1src_copy = copy_rtx (i1src);
> 
> but why not just
> 
>  if (i0_feeds_i1_n && added_sets_2 && i1_feeds_i2_n)
> 
> i.e. make a copy if substituting I0 will clobber I1SRC and I1SRC will be re- 
> substituted in I2PAT?

I think you are right, if !i1_feeds_i2_n then the copy is not needed,
because it will not be used.  If !i0dest_in_i0src, I think
the copy is not strictily needed, because it shouldn't matter whether
i0dest is replaced with i0src just once or more than once, but if you
prefer the if (i0_feeds_i1_n && added_sets_2 && i1_feeds_i2_n)
condition, I can bootstrap/regtest it with that.

> 
> > The other issue is that if we are to apply more than 
> > one substitution on i2pat and i0dest_in_i0src, then we need to pass
> > 1 as last argument to the first subst in order to avoid unwanted
> > rtl sharing (which again can lead to self-referential rtl).
> > Another issue is that if all of i0_feeds_i2_n, i0_feeds_i1_n and
> > i1_feeds_i2_n is true, then we'd be substituting i0dest with i0src
> > in i2pat twice.
> 
> This part looks OK to me.
> 
> 
> 	* combine.c (try_combine): Fix formatting issues and a pasto.

Yeah, the

> -      newpat = subst (newpat, i0dest, i0src, 0,
> -                   i0_feeds_i1_n && i0dest_in_i0src);
> +      newpat = subst (newpat, i0dest, i0src, 0, 0);

is what I came across too and it surprised me, but I decided it
doesn't break anything, just is inefficient.  But you're right it is better
to fix it to make the code more readable.

	Jakub
Eric Botcazou - Nov. 2, 2010, 9:01 p.m.
> I think you are right, if !i1_feeds_i2_n then the copy is not needed,
> because it will not be used.  If !i0dest_in_i0src, I think
> the copy is not strictily needed, because it shouldn't matter whether
> i0dest is replaced with i0src just once or more than once, but if you
> prefer the if (i0_feeds_i1_n && added_sets_2 && i1_feeds_i2_n)
> condition, I can bootstrap/regtest it with that.

Yes, I think this is easier to understand that way.  OK with this change if it 
successfully completes the testing cycle.

> > 	* combine.c (try_combine): Fix formatting issues and a pasto.
>
> Yeah, the
>
> > -      newpat = subst (newpat, i0dest, i0src, 0,
> > -                   i0_feeds_i1_n && i0dest_in_i0src);
> > +      newpat = subst (newpat, i0dest, i0src, 0, 0);
>
> is what I came across too and it surprised me, but I decided it
> doesn't break anything, just is inefficient.  But you're right it is better
> to fix it to make the code more readable.

My opinion as well, the pasto can confuse the reader (it did with me :-)

Patch

Index: combine.c
===================================================================
--- combine.c	(revision 166059)
+++ combine.c	(working copy)
@@ -3071,23 +3071,23 @@  try_combine (rtx i3, rtx i2, rtx i1, rtx
 	}
 
       n_occurrences = 0;		/* `subst' counts here */
-
-      /* If I1 feeds into I2 and I1DEST is in I1SRC, we need to make a
-	 unique copy of I2SRC each time we substitute it to avoid
-	 self-referential rtl.  */
-
       subst_low_luid = DF_INSN_LUID (i2);
+
+      /* If I1 feeds into I2 and I1DEST is in I1SRC, we need to make a unique
+	 copy of I2SRC each time we substitute it, in order to avoid creating
+	 self-referential RTL when we will be substituting I1SRC for I1DEST
+	 later.  Likewise if I0 feeds into I2 and I0DEST is in I0SRC.  */
       newpat = subst (PATTERN (i3), i2dest, i2src, 0,
-		      ((i1_feeds_i2_n && i1dest_in_i1src)
-		       || (i0_feeds_i2_n && i0dest_in_i0src)));
+		      (i1_feeds_i2_n && i1dest_in_i1src)
+		      || (i0_feeds_i2_n && i0dest_in_i0src));
       substed_i2 = 1;
 
-      /* Record whether i2's body now appears within i3's body.  */
+      /* Record whether I2's body now appears within I3's body.  */
       i2_is_used = n_occurrences;
     }
 
-  /* If we already got a failure, don't try to do more.  Otherwise,
-     try to substitute in I1 if we have it.  */
+  /* If we already got a failure, don't try to do more.  Otherwise, try to
+     substitute I1 if we have it.  */
 
   if (i1 && GET_CODE (newpat) != CLOBBER)
     {
@@ -3098,10 +3098,10 @@  try_combine (rtx i3, rtx i2, rtx i1, rtx
 	   && i1_feeds_i2_n
 	   && dead_or_set_p (i2, i1dest)
 	   && !reg_overlap_mentioned_p (i1dest, newpat))
-	  /* Before we can do this substitution, we must redo the test done
-	     above (see detailed comments there) that ensures  that I1DEST
-	     isn't mentioned in any SETs in NEWPAT that are field assignments.  */
-          || !combinable_i3pat (NULL_RTX, &newpat, i1dest, NULL_RTX, NULL_RTX,
+	   /* Before we can do this substitution, we must redo the test done
+	      above (see detailed comments there) that ensures I1DEST isn't
+	      mentioned in any SETs in NEWPAT that are field assignments.  */
+	  || !combinable_i3pat (NULL_RTX, &newpat, i1dest, NULL_RTX, NULL_RTX,
 				0, 0, 0))
 	{
 	  undo_all ();
@@ -3110,18 +3110,28 @@  try_combine (rtx i3, rtx i2, rtx i1, rtx
 
       n_occurrences = 0;
       subst_low_luid = DF_INSN_LUID (i1);
+
+      /* If I0 feeds into I1 and I0DEST is in I0SRC, we need to make a unique
+	 copy of I1SRC each time we substitute it, in order to avoid creating
+	 self-referential RTL when we will be substituting I0SRC for I0DEST
+	 later.  */
       newpat = subst (newpat, i1dest, i1src, 0,
 		      i0_feeds_i1_n && i0dest_in_i0src);
       substed_i1 = 1;
+
+      /* Record whether I1's body now appears within I3's body.  */
       i1_is_used = n_occurrences;
     }
+
+  /* Likewise for I0 if we have it.  */
+
   if (i0 && GET_CODE (newpat) != CLOBBER)
     {
       if ((FIND_REG_INC_NOTE (i0, NULL_RTX) != 0
 	   && ((i0_feeds_i2_n && dead_or_set_p (i2, i0dest))
 	       || (i0_feeds_i1_n && dead_or_set_p (i1, i0dest)))
 	   && !reg_overlap_mentioned_p (i0dest, newpat))
-          || !combinable_i3pat (NULL_RTX, &newpat, i0dest, NULL_RTX, NULL_RTX,
+	  || !combinable_i3pat (NULL_RTX, &newpat, i0dest, NULL_RTX, NULL_RTX,
 				0, 0, 0))
 	{
 	  undo_all ();
@@ -3130,8 +3140,7 @@  try_combine (rtx i3, rtx i2, rtx i1, rtx
 
       n_occurrences = 0;
       subst_low_luid = DF_INSN_LUID (i0);
-      newpat = subst (newpat, i0dest, i0src, 0,
-		      i0_feeds_i1_n && i0dest_in_i0src);
+      newpat = subst (newpat, i0dest, i0src, 0, 0);
       substed_i0 = 1;
     }