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 3E6D83857421 for ; Mon, 26 Jul 2021 19:10:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3E6D83857421 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 DC88B31B; Mon, 26 Jul 2021 12:10:16 -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 688733F66F; Mon, 26 Jul 2021 12:10:16 -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 3/7] ifcvt: Improve costs handling for noce_convert_multiple. References: <20210625160905.23786-1-rdapp@linux.ibm.com> <20210625160905.23786-4-rdapp@linux.ibm.com> <74d71e7f-c3ac-1e0f-ac04-339f44674ded@linux.ibm.com> Date: Mon, 26 Jul 2021 20:10:15 +0100 In-Reply-To: <74d71e7f-c3ac-1e0f-ac04-339f44674ded@linux.ibm.com> (Robin Dapp's message of "Thu, 22 Jul 2021 14:07:04 +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:10:18 -0000 Robin Dapp writes: >> It looks like this is an existing (potential) problem, >> but default_noce_conversion_profitable_p uses seq_cost, which in turn >> uses insn_cost. And insn_cost has an optional target hook behind it, >> which allows for costing based on insn attributes etc. For a true >> apples-with-apples comparison we should use insn_cost here too. > > Good point, fixed that. > >> I think the detail that COSTS_N_INSNS (2) is the default is useful here. >> (In other words, I'd forgotten by the time I'd poked around other bits of >> ifcvt and was about to ask why we didn't cost the condition =E2=80=9Cpro= perly=E2=80=9D.) >> So how about something like: >>=20 >> The original costs already include a base cost of COSTS_N_INSNS (2): >> one instruction for the compare (which we will be needing either way) >> and one instruction for the branch. > > Yes, this is much clearer. I went with that wording in the attached v2. > > Regards > Robin > > From 54508fa00fbee082fa62f4e1a8167f477938e781 Mon Sep 17 00:00:00 2001 > From: Robin Dapp > Date: Wed, 27 Nov 2019 13:46:17 +0100 > Subject: [PATCH v2 3/7] ifcvt: Improve costs handling for > noce_convert_multiple. > > When noce_convert_multiple is called the original costs are not yet > initialized. Therefore, up to now, costs were only ever unfairly > compared against COSTS_N_INSNS (2). This would lead to > default_noce_conversion_profitable_p () rejecting all but the most > contrived of sequences. > > This patch temporarily initializes the original costs by counting > adding costs for all sets inside the then_bb. OK, thanks. Richard > --- > gcc/ifcvt.c | 31 +++++++++++++++++++++++++++---- > 1 file changed, 27 insertions(+), 4 deletions(-) > > diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c > index f9d4478ec18..91b54dbea9c 100644 > --- a/gcc/ifcvt.c > +++ b/gcc/ifcvt.c > @@ -3404,14 +3404,17 @@ noce_convert_multiple_sets (struct noce_if_info *= if_info) > (SET (REG) (REG)) insns suitable for conversion to a series > of conditional moves. Also check that we have more than one set > (other routines can handle a single set better than we would), and > - fewer than PARAM_MAX_RTL_IF_CONVERSION_INSNS sets. */ > + fewer than PARAM_MAX_RTL_IF_CONVERSION_INSNS sets. While going > + through the insns store the sum of their potential costs in COST. */ >=20=20 > static bool > -bb_ok_for_noce_convert_multiple_sets (basic_block test_bb) > +bb_ok_for_noce_convert_multiple_sets (basic_block test_bb, unsigned *cos= t) > { > rtx_insn *insn; > unsigned count =3D 0; > unsigned param =3D param_max_rtl_if_conversion_insns; > + bool speed_p =3D optimize_bb_for_speed_p (test_bb); > + unsigned potential_cost =3D 0; >=20=20 > FOR_BB_INSNS (test_bb, insn) > { > @@ -3447,9 +3450,13 @@ bb_ok_for_noce_convert_multiple_sets (basic_block = test_bb) > if (!can_conditionally_move_p (GET_MODE (dest))) > return false; >=20=20 > + potential_cost +=3D insn_cost (insn, speed_p); > + > count++; > } >=20=20 > + *cost +=3D potential_cost; > + > /* If we would only put out one conditional move, the other strategies > this pass tries are better optimized and will be more appropriate. > Some targets want to strictly limit the number of conditional moves > @@ -3497,11 +3504,24 @@ noce_process_if_block (struct noce_if_info *if_in= fo) > to calculate a value for x. > ??? For future expansion, further expand the "multiple X" rules. */ >=20=20 > - /* First look for multiple SETS. */ > + /* First look for multiple SETS. The original costs already include > + a base cost of COSTS_N_INSNS (2): one instruction for the compare > + (which we will be needing either way) and one instruction for the > + branch. When comparing costs we want to use the branch instruction > + cost and the sets vs. the cmovs generated here. Therefore subtract > + the costs of the compare before checking. > + ??? Actually, instead of the branch instruction costs we might want > + to use COSTS_N_INSNS (BRANCH_COST ()) as in other places. */ > + > + unsigned potential_cost =3D if_info->original_cost - COSTS_N_INSNS (1); > + unsigned old_cost =3D if_info->original_cost; > if (!else_bb > && HAVE_conditional_move > - && bb_ok_for_noce_convert_multiple_sets (then_bb)) > + && bb_ok_for_noce_convert_multiple_sets (then_bb, &potential_cost)) > { > + /* Temporarily set the original costs to what we estimated so > + we can determine if the transformation is worth it. */ > + if_info->original_cost =3D potential_cost; > if (noce_convert_multiple_sets (if_info)) > { > if (dump_file && if_info->transform_name) > @@ -3509,6 +3529,9 @@ noce_process_if_block (struct noce_if_info *if_info) > if_info->transform_name); > return TRUE; > } > + > + /* Restore the original costs. */ > + if_info->original_cost =3D old_cost; > } >=20=20 > bool speed_p =3D optimize_bb_for_speed_p (test_bb);