From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id 4318E399201D for ; Thu, 22 Jul 2021 12:07:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4318E399201D Received: from pps.filterd (m0098416.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.43/8.16.0.43) with SMTP id 16MC4C5o021650; Thu, 22 Jul 2021 08:07:10 -0400 Received: from ppma04fra.de.ibm.com (6a.4a.5195.ip4.static.sl-reverse.com [149.81.74.106]) by mx0b-001b2d01.pphosted.com with ESMTP id 39y6jh3s6m-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 22 Jul 2021 08:07:10 -0400 Received: from pps.filterd (ppma04fra.de.ibm.com [127.0.0.1]) by ppma04fra.de.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 16MC78k8013443; Thu, 22 Jul 2021 12:07:08 GMT Received: from b06cxnps4076.portsmouth.uk.ibm.com (d06relay13.portsmouth.uk.ibm.com [9.149.109.198]) by ppma04fra.de.ibm.com with ESMTP id 39upu89fjj-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 22 Jul 2021 12:07:08 +0000 Received: from d06av26.portsmouth.uk.ibm.com (d06av26.portsmouth.uk.ibm.com [9.149.105.62]) by b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 16MC76GN28377458 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 22 Jul 2021 12:07:06 GMT Received: from d06av26.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 57C7AAE055; Thu, 22 Jul 2021 12:07:06 +0000 (GMT) Received: from d06av26.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id BC6EAAE051; Thu, 22 Jul 2021 12:07:05 +0000 (GMT) Received: from li-926bd7cc-2dd1-11b2-a85c-f6adc0f5efec.ibm.com (unknown [9.171.28.15]) by d06av26.portsmouth.uk.ibm.com (Postfix) with ESMTPS; Thu, 22 Jul 2021 12:07:05 +0000 (GMT) Subject: Re: [PATCH 3/7] ifcvt: Improve costs handling for noce_convert_multiple. To: gcc-patches@gcc.gnu.org, richard.sandiford@arm.com References: <20210625160905.23786-1-rdapp@linux.ibm.com> <20210625160905.23786-4-rdapp@linux.ibm.com> From: Robin Dapp Message-ID: <74d71e7f-c3ac-1e0f-ac04-339f44674ded@linux.ibm.com> Date: Thu, 22 Jul 2021 14:07:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------6F37034AFDF0BD7FAE529CAD" Content-Language: en-US X-TM-AS-GCONF: 00 X-Proofpoint-GUID: kpHdxjpQastKPpE9Xy5FqQLL2D6pZfeC X-Proofpoint-ORIG-GUID: kpHdxjpQastKPpE9Xy5FqQLL2D6pZfeC X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391, 18.0.790 definitions=2021-07-22_04:2021-07-22, 2021-07-22 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 mlxlogscore=999 lowpriorityscore=0 adultscore=0 suspectscore=0 priorityscore=1501 bulkscore=0 impostorscore=0 spamscore=0 malwarescore=0 clxscore=1015 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104190000 definitions=main-2107220081 X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_MSPIKE_H2, 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, 22 Jul 2021 12:07:15 -0000 This is a multi-part message in MIME format. --------------6F37034AFDF0BD7FAE529CAD Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit > 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 “properly”.) > So how about something like: > > 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 --------------6F37034AFDF0BD7FAE529CAD Content-Type: text/x-patch; charset=UTF-8; name="v2-0003-ifcvt-Improve-costs-handling-for-noce_convert_mul.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="v2-0003-ifcvt-Improve-costs-handling-for-noce_convert_mul.pa"; filename*1="tch" >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. --- 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. */ 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 *cost) { rtx_insn *insn; unsigned count = 0; unsigned param = param_max_rtl_if_conversion_insns; + bool speed_p = optimize_bb_for_speed_p (test_bb); + unsigned potential_cost = 0; 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; + potential_cost += insn_cost (insn, speed_p); + count++; } + *cost += 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_info) to calculate a value for x. ??? For future expansion, further expand the "multiple X" rules. */ - /* 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 = if_info->original_cost - COSTS_N_INSNS (1); + unsigned old_cost = 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 = 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 = old_cost; } bool speed_p = optimize_bb_for_speed_p (test_bb); -- 2.31.1 --------------6F37034AFDF0BD7FAE529CAD--