* [PATCH] combine: Delete EQ* notes when pseudo mode changes (PR60818)
@ 2016-02-19 9:45 Segher Boessenkool
2016-02-19 10:27 ` Eric Botcazou
0 siblings, 1 reply; 9+ messages in thread
From: Segher Boessenkool @ 2016-02-19 9:45 UTC (permalink / raw)
To: gcc-patches; +Cc: Segher Boessenkool
For some modifications combine does it changes the mode of a pseudo
because of SELECT_CC_MODE. For example, it changes "unsigned >= 0" to
"unsigned != 0", and on some targets GTU and NE use different CC_MODEs.
Changing the mode of a pseudo has effect globally, so this changes any
REG_EQUAL and REG_EQUIV notes referring to the pseudo as well, which
makes them invalid. This patch finds such notes and deletes them in
these cases.
Testing on powerpc64-linux; will also test on x86_64-linux before
committing.
Segher
2016-02-19 Segher Boessenkool <segher@kernel.crashing.org>
PR 60818/rtl-optimization
* combine.c (combine_remove_reg_equal_equiv_notes_for_regno):
New function.
(try_combine): Call it when SELECT_CC_MODE makes us change the
mode of a pseudo.
---
gcc/combine.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/gcc/combine.c b/gcc/combine.c
index 24dcefa..d3b69ac 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -2553,6 +2553,32 @@ can_split_parallel_of_n_reg_sets (rtx_insn *insn, int n)
return true;
}
+/* Remove all REG_EQUAL and REG_EQUIV notes referring to REGNO. This is
+ like rtlanal's remove_reg_equal_equiv_notes_for_regno but with some big
+ differences, because combine does not keep the DF info up-to-date.
+ We do however never add or move these notes during combine, so we can
+ still use the DF info as it was at the start of combine to find all
+ such notes. */
+
+static void
+combine_remove_reg_equal_equiv_notes_for_regno (unsigned int regno)
+{
+ gcc_assert (df);
+
+ rtx reg = regno_reg_rtx[regno];
+
+ for (df_ref eq_use = DF_REG_EQ_USE_CHAIN (regno);
+ eq_use;
+ eq_use = DF_REF_NEXT_REG (eq_use))
+ {
+ rtx_insn *insn = DF_REF_INSN (eq_use);
+ rtx note = find_reg_equal_equiv_note (insn);
+
+ if (note && reg_overlap_mentioned_p (reg, XEXP (note, 0)))
+ remove_note (insn, note);
+ }
+}
+
/* Try to combine the insns I0, I1 and I2 into I3.
Here I0, I1 and I2 appear earlier than I3.
I0 and I1 can be zero; then we combine just I2 into I3, or I1 and I2 into
@@ -3184,6 +3210,8 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
newpat_dest = gen_rtx_REG (compare_mode, regno);
else
{
+ combine_remove_reg_equal_equiv_notes_for_regno (regno);
+
SUBST_MODE (regno_reg_rtx[regno], compare_mode);
newpat_dest = regno_reg_rtx[regno];
}
@@ -6638,6 +6666,12 @@ simplify_set (rtx x)
new_dest = gen_rtx_REG (compare_mode, regno);
else
{
+ /* We change the mode of the result pseudo. The expression
+ in REG_EQ* notes refering to that pseudo will likely no
+ longer make sense, so delete those notes.
+ This is PR60818. */
+ combine_remove_reg_equal_equiv_notes_for_regno (regno);
+
SUBST_MODE (regno_reg_rtx[regno], compare_mode);
new_dest = regno_reg_rtx[regno];
}
--
1.9.3
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] combine: Delete EQ* notes when pseudo mode changes (PR60818)
2016-02-19 9:45 [PATCH] combine: Delete EQ* notes when pseudo mode changes (PR60818) Segher Boessenkool
@ 2016-02-19 10:27 ` Eric Botcazou
2016-02-19 10:37 ` Segher Boessenkool
0 siblings, 1 reply; 9+ messages in thread
From: Eric Botcazou @ 2016-02-19 10:27 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
> 2016-02-19 Segher Boessenkool <segher@kernel.crashing.org>
>
> PR 60818/rtl-optimization
> * combine.c (combine_remove_reg_equal_equiv_notes_for_regno):
> New function.
> (try_combine): Call it when SELECT_CC_MODE makes us change the
> mode of a pseudo.
This looks like a big hammer to me though.
> +/* Remove all REG_EQUAL and REG_EQUIV notes referring to REGNO. This is
> + like rtlanal's remove_reg_equal_equiv_notes_for_regno but with some big
> + differences, because combine does not keep the DF info up-to-date.
> + We do however never add or move these notes during combine, so we can
> + still use the DF info as it was at the start of combine to find all
> + such notes. */
The comment is wrong, or at least confusing, since distribute_notes does deal
with REG_EQUAL and REG_EQUIV notes.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] combine: Delete EQ* notes when pseudo mode changes (PR60818)
2016-02-19 10:27 ` Eric Botcazou
@ 2016-02-19 10:37 ` Segher Boessenkool
2016-02-19 11:04 ` Eric Botcazou
0 siblings, 1 reply; 9+ messages in thread
From: Segher Boessenkool @ 2016-02-19 10:37 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
On Fri, Feb 19, 2016 at 11:27:48AM +0100, Eric Botcazou wrote:
> > 2016-02-19 Segher Boessenkool <segher@kernel.crashing.org>
> >
> > PR 60818/rtl-optimization
> > * combine.c (combine_remove_reg_equal_equiv_notes_for_regno):
> > New function.
> > (try_combine): Call it when SELECT_CC_MODE makes us change the
> > mode of a pseudo.
>
> This looks like a big hammer to me though.
Do you have something smaller in mind that still works? I'm all ears.
> > +/* Remove all REG_EQUAL and REG_EQUIV notes referring to REGNO. This is
> > + like rtlanal's remove_reg_equal_equiv_notes_for_regno but with some big
> > + differences, because combine does not keep the DF info up-to-date.
> > + We do however never add or move these notes during combine, so we can
> > + still use the DF info as it was at the start of combine to find all
> > + such notes. */
>
> The comment is wrong, or at least confusing, since distribute_notes does deal
> with REG_EQUAL and REG_EQUIV notes.
But it never adds or moves these notes. It even says so :-)
Segher
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] combine: Delete EQ* notes when pseudo mode changes (PR60818)
2016-02-19 10:37 ` Segher Boessenkool
@ 2016-02-19 11:04 ` Eric Botcazou
2016-02-19 11:44 ` Segher Boessenkool
0 siblings, 1 reply; 9+ messages in thread
From: Eric Botcazou @ 2016-02-19 11:04 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
> Do you have something smaller in mind that still works? I'm all ears.
Try to adjust the notes instead of dropping them?
> But it never adds or moves these notes. It even says so :-)
Right, but try_combine can do it, see line 4294 and below.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] combine: Delete EQ* notes when pseudo mode changes (PR60818)
2016-02-19 11:04 ` Eric Botcazou
@ 2016-02-19 11:44 ` Segher Boessenkool
2016-02-19 18:39 ` Eric Botcazou
0 siblings, 1 reply; 9+ messages in thread
From: Segher Boessenkool @ 2016-02-19 11:44 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
On Fri, Feb 19, 2016 at 12:04:17PM +0100, Eric Botcazou wrote:
> > Do you have something smaller in mind that still works? I'm all ears.
>
> Try to adjust the notes instead of dropping them?
As far as I can see that is very hard to do though, and not really worth
it -- the notes can contain an arbitrary expression in general.
Not for stage 4 certainly.
> > But it never adds or moves these notes. It even says so :-)
>
> Right, but try_combine can do it, see line 4294 and below.
That moves those notes to i2notes, and then distribute_notes drops them?
Segher
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] combine: Delete EQ* notes when pseudo mode changes (PR60818)
2016-02-19 11:44 ` Segher Boessenkool
@ 2016-02-19 18:39 ` Eric Botcazou
2016-02-19 19:15 ` Segher Boessenkool
0 siblings, 1 reply; 9+ messages in thread
From: Eric Botcazou @ 2016-02-19 18:39 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
> As far as I can see that is very hard to do though, and not really worth
> it -- the notes can contain an arbitrary expression in general.
OK, your call.
> Not for stage 4 certainly.
If we go this way, is the bug a regression? If no, why rushing the fix?
> That moves those notes to i2notes, and then distribute_notes drops them?
That's not why I understand though. The code appends i2notes to i3notes and
distribute_notes will preserve them on i3:
/* Distribute all the LOG_LINKS and REG_NOTES from I1, I2, and I3. */
if (i3notes)
distribute_notes (i3notes, i3, i3, newi2pat ? i2 : NULL,
elim_i2, elim_i1, elim_i0);
so the notes are effectively moved from I2 to I3. distribute_notes will
indeed drop the non-trivial REG_EQUAL and REG_EQUIV notes so your code is very
likely OK, it's just the wording of the comment. No big deal I agree.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] combine: Delete EQ* notes when pseudo mode changes (PR60818)
2016-02-19 18:39 ` Eric Botcazou
@ 2016-02-19 19:15 ` Segher Boessenkool
2016-02-19 19:34 ` Eric Botcazou
0 siblings, 1 reply; 9+ messages in thread
From: Segher Boessenkool @ 2016-02-19 19:15 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
On Fri, Feb 19, 2016 at 07:39:03PM +0100, Eric Botcazou wrote:
> > Not for stage 4 certainly.
>
> If we go this way, is the bug a regression? If no, why rushing the fix?
It is not a regression (it is in all open release branches already).
I can postpone it if you think that is wiser?
> > That moves those notes to i2notes, and then distribute_notes drops them?
>
> That's not why I understand though. The code appends i2notes to i3notes and
> distribute_notes will preserve them on i3:
I misread it as moving the notes from i3 to i2, ugh. It looks like we
do have a problem if i2 is a parallel with only one SET; but we already
had a problem anyway? The REG_EQ* is put on the wrong insn?
Segher
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] combine: Delete EQ* notes when pseudo mode changes (PR60818)
2016-02-19 19:15 ` Segher Boessenkool
@ 2016-02-19 19:34 ` Eric Botcazou
2016-02-19 19:59 ` Segher Boessenkool
0 siblings, 1 reply; 9+ messages in thread
From: Eric Botcazou @ 2016-02-19 19:34 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
> It is not a regression (it is in all open release branches already).
> I can postpone it if you think that is wiser?
I do think that the combiner is one of those pieces of code that you ought not
to touch unless you really need to.
> I misread it as moving the notes from i3 to i2, ugh. It looks like we
> do have a problem if i2 is a parallel with only one SET; but we already
> had a problem anyway? The REG_EQ* is put on the wrong insn?
Possibly indeed, we might need to filter them out during the move.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] combine: Delete EQ* notes when pseudo mode changes (PR60818)
2016-02-19 19:34 ` Eric Botcazou
@ 2016-02-19 19:59 ` Segher Boessenkool
0 siblings, 0 replies; 9+ messages in thread
From: Segher Boessenkool @ 2016-02-19 19:59 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
On Fri, Feb 19, 2016 at 08:34:03PM +0100, Eric Botcazou wrote:
> > It is not a regression (it is in all open release branches already).
> > I can postpone it if you think that is wiser?
>
> I do think that the combiner is one of those pieces of code that you ought not
> to touch unless you really need to.
Oh I agree, which is why I throw all patches through testing on a zillion
different archs, and bootstraps on a few.
I'll postpone this patch to GCC 7, stage 4 is too late for it.
Segher
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-02-19 19:59 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-19 9:45 [PATCH] combine: Delete EQ* notes when pseudo mode changes (PR60818) Segher Boessenkool
2016-02-19 10:27 ` Eric Botcazou
2016-02-19 10:37 ` Segher Boessenkool
2016-02-19 11:04 ` Eric Botcazou
2016-02-19 11:44 ` Segher Boessenkool
2016-02-19 18:39 ` Eric Botcazou
2016-02-19 19:15 ` Segher Boessenkool
2016-02-19 19:34 ` Eric Botcazou
2016-02-19 19:59 ` Segher Boessenkool
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).