On Sun, 2021-03-21 at 13:19 +0000, Richard Sandiford wrote:
> Ilya Leoshkevich <i...@linux.ibm.com> writes:
> > Bootstrapped and regtested on x86_64-redhat-linux, ppc64le-redhat-
> > linux
> > and s390x-redhat-linux. Ok for master?
>
> Given what was said downthread, I agree we should fix this for GCC
> 11.
> Sorry for missing this problem in the initial review.
>
> > Commit efb6bc55a93a ("fwprop: Allow (subreg (mem))
> > simplifications")
> > introduced a check that was supposed to look at the propagated
> > def's
> > number of uses. It uses insn_info::num_uses (), which in reality
> > returns the number of uses def's insn has. The whole change
> > therefore
> > works only by accident.
> >
> > Fix by looking at def_info's uses instead of insn_info's uses.
> > This
> > requires passing around def_info instead of insn_info.
> >
> > gcc/ChangeLog:
> >
> > 2021-03-02 Ilya Leoshkevich <i...@linux.ibm.com>
> >
> > * fwprop.c (def_has_single_use_p): New function.
> > (fwprop_propagation::fwprop_propagation): Look at
> > def_info's uses.
> > (try_fwprop_subst_note): Use def_info instead of insn_info.
> > (try_fwprop_subst_pattern): Likewise.
> > (try_fwprop_subst_notes): Likewise.
> > (try_fwprop_subst): Likewise.
> > (forward_propagate_subreg): Likewise.
> > (forward_propagate_and_simplify): Likewise.
> > (forward_propagate_into): Likewise.
> > * iterator-utils.h (single_element_p): New function.
> > ---
> > gcc/fwprop.c | 89 ++++++++++++++++++++++++++--------------
> > ----
> > gcc/iterator-utils.h | 10 +++++
> > 2 files changed, 62 insertions(+), 37 deletions(-)
> >
> > diff --git a/gcc/fwprop.c b/gcc/fwprop.c
> > index 4b8a554e823..478dcdd96cc 100644
> > --- a/gcc/fwprop.c
> > +++ b/gcc/fwprop.c
> > @@ -175,7 +175,7 @@ namespace
> > static const uint16_t CONSTANT = FIRST_SPARE_RESULT << 1;
> > static const uint16_t PROFITABLE = FIRST_SPARE_RESULT << 2;
> >
> > - fwprop_propagation (insn_info *, insn_info *, rtx, rtx);
> > + fwprop_propagation (insn_info *, def_info *, rtx, rtx);
>
> use->def () returns a set_info *, and since you want set_info stuff,
> I think it would probably be better to pass around a set_info *
> instead.
> (Let's keep the variable names the same though. “def” is still
> accurate
> and IMO the natural choice.)
>
> > @@ -191,13 +191,27 @@ namespace
> > };
> > }
> >
> > -/* Prepare to replace FROM with TO in INSN. */
> > +/* Return true if DEF has a single non-debug non-phi use. */
> > +
> > +static bool
> > +def_has_single_use_p (def_info *def)
> > +{
> > + if (!is_a<set_info *> (def))
> > + return false;
> > +
> > + set_info *set = as_a<set_info *> (def);
> > +
> > + return single_element_p (set->nondebug_insn_uses ())
> > + && !set->has_phi_uses ();
>
> I think instead we should add:
>
> // If exactly one nondebug instruction uses the set's result,
> return
> // the use by that instruction, otherwise return null.
> use_info *single_nondebug_insn_use () const;
>
> // If there is exactly one nondebug use of the set's result,
> // return that use, otherwise return null. The use might be in
> // instruction or a phi node.
> use_info *single_nondebug_use () const;
>
> before the declaration of set_info::is_local_to_ebb.
>
> > +}
> > +
> > +/* Prepare to replace FROM with TO in USE_INSN. */
> >
> > fwprop_propagation::fwprop_propagation (insn_info *use_insn,
> > - insn_info *def_insn, rtx
> > from, rtx to)
> > + def_info *def, rtx from,
> > rtx to)
> > : insn_propagation (use_insn->rtl (), from, to),
> > - single_use_p (def_insn->num_uses () == 1),
> > - single_ebb_p (use_insn->ebb () == def_insn->ebb ())
> > + single_use_p (def_has_single_use_p (def)),
> > + single_ebb_p (use_insn->ebb () == def->insn ()->ebb ())
>
> Just def->ebb ()
>
> > @@ -538,7 +554,7 @@ try_fwprop_subst_pattern (obstack_watermark
> > &attempt, insn_change &use_change,
> > {
> > if ((REG_NOTE_KIND (note) == REG_EQUAL
> > || REG_NOTE_KIND (note) == REG_EQUIV)
> > - && try_fwprop_subst_note (use_insn, def_insn, note,
> > + && try_fwprop_subst_note (use_insn, def, note,
> > dest, src, false) < 0)
>
> Very minor, sorry, but this now fits on one line.
>
> > @@ -584,10 +600,11 @@ try_fwprop_subst_notes (insn_info *use_insn,
> > insn_info *def_insn,
> > Return true on success, otherwise leave USE_INSN unchanged. */
> >
> > static bool
> > -try_fwprop_subst (use_info *use, insn_info *def_insn,
> > +try_fwprop_subst (use_info *use, def_info *def,
> > rtx *loc, rtx dest, rtx src)
>
> Same here.
>
> Thanks,
> Richard
Thanks for reviewing! I'm currently regtesting a v2.
One thing though: I don't think we need single_nondebug_use() for this
fix, only single_nondebug_insn_use() - the fwprop check that I'm now
using is def->single_nondebug_insn_use () && !def->has_phi_uses ().
Do you still want me to add single_nondebug_use() for completeness in
this patch, or would it be better to add it later when it's actually
needed?
