Ilya Leoshkevich <i...@linux.ibm.com> writes:
> 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?
I was thinking that the fwprop.c code would use
def->single_nondebug_use () instead of
def->single_nondebug_insn_use () && !def->has_phi_uses ().
What the fwprop.c code is asking is: ignoring debug instructions,
is this value used only in one place? That seems like it might
be a common query and so I'd rather have a single function for it.
(It happens that, in this context, we already know that any single
user would be the use that we already looked at. But that's OK. :-))
I'd suggested adding def->single_nondebug_insn_use () too because
I was imagining that def->single_nondebug_use () would use it
internally.
FWIW, it would also be possible to query directly whether a given
use_info is the only non-debug use, if that's easier/more natural.
Thanks,
Richard
