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 A86E63857421 for ; Mon, 26 Jul 2021 19:08:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A86E63857421 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 5206131B; Mon, 26 Jul 2021 12:08:19 -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 D095B3F66F; Mon, 26 Jul 2021 12:08:18 -0700 (PDT) From: Richard Sandiford To: Robin Dapp Mail-Followup-To: Robin Dapp , gcc-patches@gcc.gnu.org, richard.sandiford@arm.com Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH 1/7] ifcvt: Check if cmovs are needed. References: <20210625160905.23786-1-rdapp@linux.ibm.com> <20210625160905.23786-2-rdapp@linux.ibm.com> <399e9ada-8a0a-e95b-f037-b3f3cb8a6c48@linux.ibm.com> Date: Mon, 26 Jul 2021 20:08:17 +0100 In-Reply-To: <399e9ada-8a0a-e95b-f037-b3f3cb8a6c48@linux.ibm.com> (Robin Dapp's message of "Thu, 22 Jul 2021 14:06:46 +0200") 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=-12.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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, 26 Jul 2021 19:08:21 -0000 Robin Dapp writes: > Hi Richard, > > thanks for going through the patch set. > >> A hash_set might be simpler, given that the code only enters insns >> for which the bool is false. =E2=80=9Crtx_insn *=E2=80=9D would be bett= er than rtx. > > Right, changed that. > >> Do you mean the sets might be removed or that the checks might be >> removed? > > It's actually the checks that are removed. I clarified and amended the=20 > comments. > >> The patch is quite hard to review on its own, since nothing actually >> uses this variable. It's also not obvious how the >> reg_overlap_mentioned_p code works if the old target is referenced >> later. >>=20 >> Could you refactor the series a bit so that each patch is >> self-contained? It's OK if that means fewer patches. > The attached v2 makes use of need_cmov now, I hope this makes it a bit=20 > clearer. Regarding the reg_overlap_mentioned_p: it is used to detect=20 > the swap idioms as well as when a cmov destination is used in the=20 > condition as well. If needed this could be two separate patches (most=20 > of the second patch would be comments, though). Thanks for the update. No need for further splitting, this looks like a nice self-contained patch as it is. > diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c > index 017944f4f79..a5e55456d88 100644 > --- a/gcc/ifcvt.c > +++ b/gcc/ifcvt.c > @@ -98,6 +98,7 @@ static int dead_or_predicable (basic_block, basic_block= , basic_block, > edge, int); > static void noce_emit_move_insn (rtx, rtx); > static rtx_insn *block_has_only_trap (basic_block); > +static void check_need_cmovs (basic_block, hash_set *); > > /* Count the number of non-jump active insns in BB. */ >=20=20 > @@ -3203,6 +3204,10 @@ noce_convert_multiple_sets (struct noce_if_info *i= f_info) > auto_vec unmodified_insns; > int count =3D 0; >=20=20 > + hash_set need_no_cmov; > + > + check_need_cmovs (then_bb, &need_no_cmov); > + > FOR_BB_INSNS (then_bb, insn) > { > /* Skip over non-insns. */ > @@ -3213,26 +3218,47 @@ noce_convert_multiple_sets (struct noce_if_info *= if_info) > gcc_checking_assert (set); >=20=20 > rtx target =3D SET_DEST (set); > - rtx temp =3D gen_reg_rtx (GET_MODE (target)); > + rtx temp; > rtx new_val =3D SET_SRC (set); > rtx old_val =3D target; >=20=20 > - /* If we were supposed to read from an earlier write in this block, > - we've changed the register allocation. Rewire the read. While > - we are looking, also try to catch a swap idiom. */ > - for (int i =3D count - 1; i >=3D 0; --i) > - if (reg_overlap_mentioned_p (new_val, targets[i])) > - { > - /* Catch a "swap" style idiom. */ > - if (find_reg_note (insn, REG_DEAD, new_val) !=3D NULL_RTX) > - /* The write to targets[i] is only live until the read > - here. As the condition codes match, we can propagate > - the set to here. */ > - new_val =3D SET_SRC (single_set (unmodified_insns[i])); > - else > - new_val =3D temporaries[i]; > - break; > - } Don't we still need this code (without the REG_DEAD handling) for the case in which=E2=80=A6 > + /* As we are transforming > + if (x > y) > + { > + a =3D b; > + c =3D d; > + } > + into > + a =3D (x > y) ... > + c =3D (x > y) ... > + > + we potentially check x > y before every set. > + Even though the check might be removed by subsequent passes, this means > + that we cannot transform > + if (x > y) > + { > + x =3D y; > + ... > + } > + into > + x =3D (x > y) ... > + ... > + since this would invalidate x and the following to-be-removed checks. > + Therefore we introduce a temporary every time we are about to > + overwrite a variable used in the check. Costing of a sequence with > + these is going to be inaccurate so only use temporaries when > + needed. */ > + if (reg_overlap_mentioned_p (target, cond)) > + temp =3D gen_reg_rtx (GET_MODE (target)); =E2=80=A6this code triggers? I don't see otherwise how later uses of x wou= ld pick up =E2=80=9Ctemp=E2=80=9D instead of the original target. E.g. suppos= e we had: if (x > y) { x =3D =E2=80=A6; z =3D x; // x does not die here } Without the loop, it looks like z would pick up the old value of x (used in the comparison) instead of the new one. > + else > + temp =3D target; > + > + /* We have identified swap-style idioms in check_need_cmovs. A no= rmal > + set will need to be a cmov while the first instruction of a swap-style > + idiom can be a regular move. This helps with costing. */ > + bool need_cmov =3D true; > + if (need_no_cmov.contains (insn)) > + need_cmov =3D false; Would be simpler as: bool need_cmov =3D !need_no_cmov.contains (insn); >=20=20 > /* If we had a non-canonical conditional jump (i.e. one where > the fallthrough is to the "else" case) we need to reverse > @@ -3275,9 +3301,22 @@ noce_convert_multiple_sets (struct noce_if_info *i= f_info) > old_val =3D lowpart_subreg (dst_mode, old_val, src_mode); > } >=20=20 > - /* Actually emit the conditional move. */ > - rtx temp_dest =3D noce_emit_cmove (if_info, temp, cond_code, > - x, y, new_val, old_val); > + rtx temp_dest =3D NULL_RTX; > + > + if (need_cmov) > + { > + /* Actually emit the conditional move. */ > + temp_dest =3D noce_emit_cmove (if_info, temp, cond_code, > + x, y, new_val, old_val); > + } > + else > + { > + if (if_info->then_else_reverse) > + noce_emit_move_insn (temp, old_val); > + else > + noce_emit_move_insn (temp, new_val); > + temp_dest =3D temp; > + } >=20=20 > /* If we failed to expand the conditional move, drop out and don't > try to continue. */ I think this comment and the associated null check belong in the =E2=80=9Cif (need_cmov)=E2=80=9D > @@ -3808,6 +3847,65 @@ check_cond_move_block (basic_block bb, > return TRUE; > } >=20=20 > +/* Find local swap-style idioms in BB and mark the first insn (1) > + that is only a temporary as not needing a conditional move as > + it is going to be dead afterwards anyway. > + > + (1) int tmp =3D a; > + a =3D b; > + b =3D tmp; > + > + ifcvt > + --> > + > + load tmp,a > + cmov a,b > + cmov b,tmp */ > + > +static void > +check_need_cmovs (basic_block bb, hash_set *need_no_cmov) > +{ > + rtx_insn *insn; > + int count =3D 0; > + auto_vec insns; > + auto_vec dests; > + > + /* Iterate over all SETs, storing the destinations > + in DEST. If we hit a SET that reads from a destination > + that we have seen before and the corresponding register > + is dead afterwards, it must be a swap. */ This is probably pedantic, but I guess it could also be a missed forward-propagation opportunity. E.g. there's no reason in principle why we couldn't see: int tmp =3D a; b =3D tmp; // tmp dies here The code handles that case correctly, but it isn't a swap. > + FOR_BB_INSNS (bb, insn) > + { > + rtx set, src, dest; > + > + if (!active_insn_p (insn)) > + continue; > + > + set =3D single_set (insn); > + if (set =3D=3D NULL_RTX) > + continue; > + > + src =3D SET_SRC (set); > + dest =3D SET_DEST (set); > + > + /* Check if the current SET's source is the same > + as any previously seen destination. > + This is quadratic but the number of insns in BB > + is bounded by PARAM_MAX_RTL_IF_CONVERSION_INSNS. */ > + for (int i =3D count - 1; i >=3D 0; --i) > + { > + if (reg_overlap_mentioned_p (src, dests[i]) > + && find_reg_note (insn, REG_DEAD, src) !=3D NULL_RTX) > + need_no_cmov->add (insns[i]); > + } Minor formatting nit, sorry, but the braces aren't needed. This is really a comment about the existing swap recognition code, but I think the reg_overlap_mentioned_p check would be more obviously correct if we guard the =E2=80=9Cfor=E2=80=9D loop with: if (REG_P (src)) This guarantees that any previous instructions are equal to SRC or subregs of it. I guess it might be more efficient too. Thanks, Richard