From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id E47B63858D29; Mon, 22 Mar 2021 18:23:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E47B63858D29 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6CC1E113E; Mon, 22 Mar 2021 11:23:10 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.126]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id AF38A3F718; Mon, 22 Mar 2021 11:23:09 -0700 (PDT) From: Richard Sandiford To: Ilya Leoshkevich Mail-Followup-To: Ilya Leoshkevich , gcc-patches@gcc.gnu.org, Andreas Krebbel , Stefan Liebler , richard.sandiford@arm.com Cc: gcc-patches@gcc.gnu.org, Andreas Krebbel , Stefan Liebler Subject: Re: [PATCH] fwprop: Fix single_use_p calculation References: <20210302223726.135359-1-iii@linux.ibm.com> Date: Mon, 22 Mar 2021 18:23:08 +0000 In-Reply-To: (Ilya Leoshkevich's message of "Mon, 22 Mar 2021 15:45:16 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, BODY_8BITS, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 22 Mar 2021 18:23:12 -0000 Ilya Leoshkevich writes: > On Sun, 2021-03-21 at 13:19 +0000, Richard Sandiford wrote: >> Ilya Leoshkevich writes: >> > Bootstrapped and regtested on x86_64-redhat-linux, ppc64le-redhat- >> > linux >> > and s390x-redhat-linux.=C2=A0 Ok for master? >>=20 >> Given what was said downthread, I agree we should fix this for GCC >> 11. >> Sorry for missing this problem in the initial review. >>=20 >> > Commit efb6bc55a93a ("fwprop: Allow (subreg (mem)) >> > simplifications") >> > introduced a check that was supposed to look at the propagated >> > def's >> > number of uses.=C2=A0 It uses insn_info::num_uses (), which in reality >> > returns the number of uses def's insn has.=C2=A0 The whole change >> > therefore >> > works only by accident. >> >=20 >> > Fix by looking at def_info's uses instead of insn_info's uses.=C2=A0 >> > This >> > requires passing around def_info instead of insn_info. >> >=20 >> > gcc/ChangeLog: >> >=20 >> > 2021-03-02=C2=A0 Ilya Leoshkevich=C2=A0 >> >=20 >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* fwprop.c (def_has_si= ngle_use_p): New function. >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(fwprop_propagation::f= wprop_propagation): Look at >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0def_info's uses. >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(try_fwprop_subst_note= ): Use def_info instead of insn_info. >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(try_fwprop_subst_patt= ern): Likewise. >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(try_fwprop_subst_note= s): Likewise. >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(try_fwprop_subst): Li= kewise. >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(forward_propagate_sub= reg): Likewise. >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(forward_propagate_and= _simplify): Likewise. >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(forward_propagate_int= o): Likewise. >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* iterator-utils.h (si= ngle_element_p): New function. >> > --- >> > =C2=A0gcc/fwprop.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | 8= 9 ++++++++++++++++++++++++++-------------- >> > ---- >> > =C2=A0gcc/iterator-utils.h | 10 +++++ >> > =C2=A02 files changed, 62 insertions(+), 37 deletions(-) >> >=20 >> > 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 >> > =C2=A0=C2=A0=C2=A0=C2=A0 static const uint16_t CONSTANT =3D FIRST_SPAR= E_RESULT << 1; >> > =C2=A0=C2=A0=C2=A0=C2=A0 static const uint16_t PROFITABLE =3D FIRST_SP= ARE_RESULT << 2; >> > =C2=A0 >> > -=C2=A0=C2=A0=C2=A0 fwprop_propagation (insn_info *, insn_info *, rtx,= rtx); >> > +=C2=A0=C2=A0=C2=A0 fwprop_propagation (insn_info *, def_info *, rtx, = rtx); >>=20 >> 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.=C2=A0 =E2=80=9Cdef=E2=80= =9D is still >> accurate >> and IMO the natural choice.) >>=20 >> > @@ -191,13 +191,27 @@ namespace >> > =C2=A0=C2=A0 }; >> > =C2=A0} >> > =C2=A0 >> > -/* Prepare to replace FROM with TO in INSN.=C2=A0 */ >> > +/* Return true if DEF has a single non-debug non-phi use.=C2=A0 */ >> > + >> > +static bool >> > +def_has_single_use_p (def_info *def) >> > +{ >> > +=C2=A0 if (!is_a (def)) >> > +=C2=A0=C2=A0=C2=A0 return false; >> > + >> > +=C2=A0 set_info *set =3D as_a (def); >> > + >> > +=C2=A0 return single_element_p (set->nondebug_insn_uses ()) >> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 && !set->has_phi_uses (); >>=20 >> I think instead we should add: >>=20 >> =C2=A0 // If exactly one nondebug instruction uses the set's result, >> return >> =C2=A0 // the use by that instruction, otherwise return null. >> =C2=A0 use_info *single_nondebug_insn_use () const; >>=20 >> =C2=A0 // If there is exactly one nondebug use of the set's result, >> =C2=A0 // return that use, otherwise return null.=C2=A0 The use might be= in >> =C2=A0 // instruction or a phi node. >> =C2=A0 use_info *single_nondebug_use () const; >>=20 >> before the declaration of set_info::is_local_to_ebb. >>=20 >> > +} >> > + >> > +/* Prepare to replace FROM with TO in USE_INSN.=C2=A0 */ >> > =C2=A0 >> > =C2=A0fwprop_propagation::fwprop_propagation (insn_info *use_insn, >> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0insn_info *def_insn, rtx >> > from, rtx to) >> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0def_info *def, rtx from, >> > rtx to) >> > =C2=A0=C2=A0 : insn_propagation (use_insn->rtl (), from, to), >> > -=C2=A0=C2=A0=C2=A0 single_use_p (def_insn->num_uses () =3D=3D 1), >> > -=C2=A0=C2=A0=C2=A0 single_ebb_p (use_insn->ebb () =3D=3D def_insn->eb= b ()) >> > +=C2=A0=C2=A0=C2=A0 single_use_p (def_has_single_use_p (def)), >> > +=C2=A0=C2=A0=C2=A0 single_ebb_p (use_insn->ebb () =3D=3D def->insn ()= ->ebb ()) >>=20 >> Just def->ebb () >>=20 >> > @@ -538,7 +554,7 @@ try_fwprop_subst_pattern (obstack_watermark >> > &attempt, insn_change &use_change, >> > =C2=A0=C2=A0=C2=A0=C2=A0 { >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if ((REG_NOTE_KIND (note) =3D=3D = REG_EQUAL >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 || REG_NO= TE_KIND (note) =3D=3D REG_EQUIV) >> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 && try_fwprop_subst_= note (use_insn, def_insn, note, >> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 && try_fwprop_subst_= note (use_insn, def, note, >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 dest, sr= c, false) < 0) >>=20 >> Very minor, sorry, but this now fits on one line. >>=20 >> > @@ -584,10 +600,11 @@ try_fwprop_subst_notes (insn_info *use_insn, >> > insn_info *def_insn, >> > =C2=A0=C2=A0=C2=A0 Return true on success, otherwise leave USE_INSN un= changed.=C2=A0 */ >> > =C2=A0 >> > =C2=A0static bool >> > -try_fwprop_subst (use_info *use, insn_info *def_insn, >> > +try_fwprop_subst (use_info *use, def_info *def, >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 rtx *loc, rtx dest, rtx src) >>=20 >> Same here. >>=20 >> 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