diff mbox

combine: Delete EQ* notes when pseudo mode changes (PR60818)

Message ID 71ce2561d2dd574e38b062a094f4ce41cac21a84.1455873752.git.segher@kernel.crashing.org
State New
Headers show

Commit Message

Segher Boessenkool Feb. 19, 2016, 9:45 a.m. UTC 
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(+)

Comments

Eric Botcazou Feb. 19, 2016, 10:27 a.m. UTC | #1 
> 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.
Segher Boessenkool Feb. 19, 2016, 10:37 a.m. UTC | #2 
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
Eric Botcazou Feb. 19, 2016, 11:04 a.m. UTC | #3 
> 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.
Segher Boessenkool Feb. 19, 2016, 11:43 a.m. UTC | #4 
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
Eric Botcazou Feb. 19, 2016, 6:39 p.m. UTC | #5 
> 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.
Segher Boessenkool Feb. 19, 2016, 7:14 p.m. UTC | #6 
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
Eric Botcazou Feb. 19, 2016, 7:34 p.m. UTC | #7 
> 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.
Segher Boessenkool Feb. 19, 2016, 7:59 p.m. UTC | #8 
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
diff mbox

Patch

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];
 		}