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 CA54E3857C63 for ; Thu, 15 Jul 2021 20:10:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CA54E3857C63 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 6D4C631B; Thu, 15 Jul 2021 13:10:40 -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 E98AC3F694; Thu, 15 Jul 2021 13:10:39 -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> Date: Thu, 15 Jul 2021 21:10:38 +0100 In-Reply-To: <20210625160905.23786-2-rdapp@linux.ibm.com> (Robin Dapp's message of "Fri, 25 Jun 2021 18:08:59 +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: Thu, 15 Jul 2021 20:10:42 -0000 Sorry for the slow review. Robin Dapp writes: > When if-converting multiple SETs and we encounter a swap-style idiom > > if (a > b) > { > tmp =3D c; // [1] > c =3D d; > d =3D tmp; > } > > ifcvt should not generate a conditional move for the instruction at > [1]. > > In order to achieve that, this patch goes through all relevant SETs > and marks the relevant instructions. This helps to evaluate costs. > > On top, only generate temporaries if the current cmov is going to > overwrite one of the comparands of the initial compare. > --- > gcc/ifcvt.c | 104 +++++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 87 insertions(+), 17 deletions(-) > > diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c > index 017944f4f79..eef6490626a 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_map *); > > /* 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_map need_cmovs; 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 better than= rtx. > + > + check_need_cmovs (then_bb, &need_cmovs); > + > FOR_BB_INSNS (then_bb, insn) > { > /* Skip over non-insns. */ > @@ -3213,26 +3218,38 @@ 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; > - } > + /* As we are transforming > + if (x > y) > + a =3D b; > + c =3D d; Looks like some missing braces here. > + into > + a =3D (x > y) ... > + c =3D (x > y) ... > + > + we potentially check x > y before every set here. > + (Even though might be removed by subsequent passes.) Do you mean the sets might be removed or that the checks might be removed? > + We cannot transform > + if (x > y) > + x =3D y; > + ... > + into > + x =3D (x > y) ... > + ... > + since this would invalidate x. 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. */ > + if (reg_overlap_mentioned_p (target, cond)) > + temp =3D gen_reg_rtx (GET_MODE (target)); > + else > + temp =3D target; > + > + bool need_cmov =3D true; > + if (need_cmovs.get (insn)) > + need_cmov =3D false; 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. Could you refactor the series a bit so that each patch is self-contained? It's OK if that means fewer patches. Thanks, Richard > /* If we had a non-canonical conditional jump (i.e. one where > the fallthrough is to the "else" case) we need to reverse > @@ -3808,6 +3825,59 @@ 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_map *need_cmov) > +{ > + rtx_insn *insn; > + int count =3D 0; > + auto_vec insns; > + auto_vec dests; > + > + 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); > + > + 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_cmov->put (insns[i], false); > + } > + } > + > + insns.safe_push (insn); > + dests.safe_push (dest); > + > + count++; > + } > +} > + > /* Given a basic block BB suitable for conditional move conversion, > a condition COND, and pointer maps THEN_VALS and ELSE_VALS containing > the register values depending on COND, emit the insns in the block as