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
