| Message ID | 201011022127.29192.ebotcazou@adacore.com |
|---|---|
| State | New |
| Headers | show |
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
> 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 :-)
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; }