Message ID | 71ce2561d2dd574e38b062a094f4ce41cac21a84.1455873752.git.segher@kernel.crashing.org |
---|---|
State | New |
Headers | show |
> 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.
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
> 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.
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
> 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.
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
> 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.
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 --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]; }