[PATCH] fwprop: Fix single_use_p calculation

[Richard Sandiford]

Ilya Leoshkevich <iii@linux.ibm.com> writes:
> On Sun, 2021-03-21 at 13:19 +0000, Richard Sandiford wrote:
>> Ilya Leoshkevich <iii@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  <iii@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