From: Eric Botcazou <ebotcazou@adacore.com>
To: Jakub Jelinek <jakub@redhat.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] Fix combiner (PRs rtl-optimization/46034, rtl-optimization/46212, rtl-optimization/46248)
Date: Tue, 02 Nov 2010 20:36:00 -0000 [thread overview]
Message-ID: <201011022127.29192.ebotcazou@adacore.com> (raw)
In-Reply-To: <20101101210903.GD29412@tyan-ft48-01.lab.bos.redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]
> 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.
--
Eric Botcazou
[-- Attachment #2: pr46034_eb.diff --]
[-- Type: text/x-diff, Size: 3709 bytes --]
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;
}
next prev parent reply other threads:[~2010-11-02 20:28 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-11-01 21:28 Jakub Jelinek
2010-11-02 20:36 ` Eric Botcazou [this message]
2010-11-02 20:51 ` Jakub Jelinek
2010-11-02 21:04 ` Eric Botcazou
2010-11-03 9:17 ` Jakub Jelinek
2010-11-03 18:03 ` Eric Botcazou
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201011022127.29192.ebotcazou@adacore.com \
--to=ebotcazou@adacore.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).