Message ID | 076a3744-f608-6f31-7244-2bf7ab06cdb1@yahoo.co.jp |
---|---|
State | Committed |
Commit | 675b1a7f113adb1d737adaf78b4fd90be7a0ed1a |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 90C923858C5E for <patchwork@sourceware.org>; Wed, 11 Jan 2023 04:21:23 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 90C923858C5E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1673410883; bh=Nyrf5xC2eoLuPy+xlXl9+tGCLltBrWBOfDszTgfdVh0=; h=Date:To:Subject:References:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=f8BGYadCGkaTXtxWnmCBZiYCAXGLj2C35CAnKh6e1rwY7hZPCAwJDRHxCPPV4ZH6h ZGanv2mznw5alU5hqmyOt5YjRVmKiEXdYVJSaa/iVqvztVc3RkAaiYUv31qdLIrstc h4vmzE6lDBKt7wFzBSX9ET6I+luPklf7MYK2ZZJ8= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from sonicconh5003-vm0.mail.kks.yahoo.co.jp (sonicconh5003-vm0.mail.kks.yahoo.co.jp [114.110.61.43]) by sourceware.org (Postfix) with ESMTPS id 0D4E33858D3C for <gcc-patches@gcc.gnu.org>; Wed, 11 Jan 2023 04:20:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0D4E33858D3C X-YMail-OSG: F7elx_EVM1m8pUz6JodqjOWTSVJzgz17GlMexsh14qXsyycrqil7wRDO8VB9PH9 rdKG8uZ4By6xufV7BcswfeGZtRJGtaooB9Pwkn_yvNRQwEPbdoSAgzHQD_TS9uqFzDhkH0h.knuf EGZpz7sDq5ZsLu.Qx_XJjPXzeCJMdRiw3GIRsIzWC9QkBWQ5mnxdw2WN9LyXTz9g.pWGkoV_qYbf VlRUbcpK.bdbIY642HEfZyuuMGmOkUVYB2eEemy1TZ2jUVe2G.Bkv8YZlsjRMgND2M55jI83nxfC Vuh6CUJVPR_ddwZjwnfIFvvtD9ri8hAkPBXabUYo4dxETWH.vBj89b2Qx_RAw.qM80KPslRvWxpO BPTpEWSNbL0O0xNHtNIq_x5SGQdbuWEHHAQzLjlqvd1_D5I3ZtIo0SZO1LolXkFyzXteW2kWVP0l KHyqdy2ZWOB3f75pE_B7WKLcOfPM6ggbjUG.F8xpLJfecZYdi5e4nhzeL4qIgJG77dk4qcm1.RnT q.M40BS_HMn8mwPh0aH.8A8wodItMg4lWw7eOWwJGe663ULpJowqpufgcYf60XbxEWHpLcv_uzBY Q7Qzj23s6pidWZxllhRonq6oaB3zvrZXjkVlrlSJ.d5MbTZ1JJh579bcnj.6KpRb6q.lodAZWo6z Zu284MeK1S2KjYmQPP1mJTMvXIgi6GYzyZYHpOsLvrCIhuRFuQe_JToDnPctOIc27Av5yXhJuhrr LZ2FLUN4cEwVzltkU4qIZf54MWZxASDhX9xTmzkuXSTEy89m3PxyXb2YJGMPrOEcVqEDgpwJ._zd wv4amd3i0Of570RFsM5GuJq_wjNWllZqckzVzYhlAvl3Efkw29Cav_RqhXeuBbHH2CD9reTyOL8H A3n9pVAFBZaLve_8YwKVyOU82DzgbPsib0l461qapQBEksmBatqhRU8lC_bNnCZc29xwVDPV5Igb fVDyuoeE- Received: from sonicgw.mail.yahoo.co.jp by sonicconh5003.mail.kks.yahoo.co.jp with HTTP; Wed, 11 Jan 2023 04:20:48 +0000 Received: by smtphe5006.mail.kks.ynwp.yahoo.co.jp (YJ Hermes SMTP Server) with ESMTPA ID 3f894a3f5b2d636d6ccb9d9c77814dcd; Wed, 11 Jan 2023 13:20:45 +0900 (JST) Message-ID: <076a3744-f608-6f31-7244-2bf7ab06cdb1@yahoo.co.jp> Date: Wed, 11 Jan 2023 13:20:42 +0900 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 To: GCC Patches <gcc-patches@gcc.gnu.org> Subject: [PATCH] ifcvt.cc: Prevent excessive if-conversion for conditional moves Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit References: <076a3744-f608-6f31-7244-2bf7ab06cdb1.ref@yahoo.co.jp> X-Spam-Status: No, score=-12.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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 <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Takayuki 'January June' Suwa via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Takayuki 'January June' Suwa <jjsuwa_sys3175@yahoo.co.jp> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
ifcvt.cc: Prevent excessive if-conversion for conditional moves
|
|
Commit Message
Takayuki 'January June' Suwa
Jan. 11, 2023, 4:20 a.m. UTC
Currently, cond_move_process_if_block() does the conversion without balancing the cost of the converted sequence with the original one, but this should be checked by calling targetm.noce_conversion_profitable_p(). Doing so allows us to provide a way based on the target-specific cost estimate, to prevent unwanted size growth due to excessive conditional moves on optimizing for size. On optimizing for speed, default_noce_conversion_profitable_p() allows plenty of headroom, so this patch has little impact. Also, if the target-specific cost estimate is accurate or allows for margins, the impact should be similarly small. gcc/ChangeLog: * ifcvt.cc (cond_move_process_if_block): Consider the result of targetm.noce_conversion_profitable_p() when replacing the original sequence with the converted one. --- gcc/ifcvt.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Comments
Hi, > On optimizing for speed, default_noce_conversion_profitable_p() allows > plenty of headroom, so this patch has little impact. > > Also, if the target-specific cost estimate is accurate or allows for > margins, the impact should be similarly small. I believe this part of ifcvt does/did not use the costing on purpose. It will generally convert more sequences than other paths that compare before and after costs since we just count the number of converted insns comparing them against the "branch costs". Similar to rtx costs they are kind of relative to a single insn but AFAIK it's not used consistently everywhere. All the major platforms have low branch costs nowadays (0 or 1?) thus we won't emit too many conditional moves here. In general I agree that we should compare costs everywhere and not just count (the costing should include the branch costs as well) but this would be a major overhaul. For your case (assuming xtensa), could you not tune xtensa_branch_cost? It is currently 3 allowing up to 4 conditional moves to be generated. optimize_function_for_speed_p is already being passed to the hook so you could make use of that and decrease branch costs when optimizing for size only. Regards Robin
On 2023/01/11 17:02, Robin Dapp wrote: > Hi, Hi! > >> On optimizing for speed, default_noce_conversion_profitable_p() allows >> plenty of headroom, so this patch has little impact. >> >> Also, if the target-specific cost estimate is accurate or allows for >> margins, the impact should be similarly small. > I believe this part of ifcvt does/did not use the costing on purpose. > It will generally convert more sequences than other paths that compare > before and after costs since we just count the number of converted > insns comparing them against the "branch costs". Similar to rtx costs > they are kind of relative to a single insn but AFAIK it's not used > consistently everywhere. All the major platforms have low branch costs > nowadays (0 or 1?) thus we won't emit too many conditional moves here. > > In general I agree that we should compare costs everywhere and not just > count (the costing should include the branch costs as well) but this would > be a major overhaul. For your case (assuming xtensa), could you not > tune xtensa_branch_cost? It is currently 3 allowing up to 4 conditional > moves to be generated. optimize_function_for_speed_p is already being > passed to the hook so you could make use of that and decrease branch > costs when optimizing for size only. > > Regards > Robin Thank you for your detailed explanation. In my case (for Xtensa), the cost of branching isn't really an issue. The actual problem (that I think) is the costs of the sequence itself before and after conversion. It is due to the fact that ifcvt's internal estimation is based on PATTERN(insn), so the instruction lengths ("length" attribute) associated with insns are not well reflected. This is especially noticeable when optimizing for size (overestimating the original cost). Currently, in addition to the patch, I have implemented the following code, and I'm confirming that it works roughly well (fine adjustments are still required). /* Return true if the instruction sequence seq is a good candidate as a replacement for the if-convertible sequence described in if_info. */ static bool xtensa_noce_conversion_profitable_p (rtx_insn *seq, struct noce_if_info *if_info) { unsigned int cost, original_cost; bool speed_p; rtx_insn *insn; speed_p = if_info->speed_p; /* of TEST_BB */ /* Estimate the cost for the replacing sequence. */ cost = 0; for (insn = seq; insn; insn = NEXT_INSN (insn)) if (active_insn_p (insn)) cost += xtensa_insn_cost (insn, speed_p); /* Short circuit and margins if optimiziing for speed. */ if (speed_p) return cost <= if_info->max_seq_cost; /* Estimate the cost for the original sequence if optimizing for size. */ original_cost = xtensa_insn_cost (if_info->jump, speed_p); speed_p = optimize_bb_for_speed_p (if_info->then_bb); FOR_BB_INSNS (if_info->then_bb, insn) if (active_insn_p (insn)) original_cost += xtensa_insn_cost (insn, speed_p); if (if_info->else_bb) { speed_p = optimize_bb_for_speed_p (if_info->else_bb); FOR_BB_INSNS (if_info->else_bb, insn) if (active_insn_p (insn)) original_cost += xtensa_insn_cost (insn, speed_p); } return cost <= original_cost; }
On 1/10/23 21:20, Takayuki 'January June' Suwa via Gcc-patches wrote: > Currently, cond_move_process_if_block() does the conversion without > balancing the cost of the converted sequence with the original one, but > this should be checked by calling targetm.noce_conversion_profitable_p(). > > Doing so allows us to provide a way based on the target-specific cost > estimate, to prevent unwanted size growth due to excessive conditional > moves on optimizing for size. > > On optimizing for speed, default_noce_conversion_profitable_p() allows > plenty of headroom, so this patch has little impact. > > Also, if the target-specific cost estimate is accurate or allows for > margins, the impact should be similarly small. > > gcc/ChangeLog: > > * ifcvt.cc (cond_move_process_if_block): > Consider the result of targetm.noce_conversion_profitable_p() > when replacing the original sequence with the converted one. This is OK for gcc-14 when stage1 opens. The only way I see including it in gcc-13 would be if it fixes a regression. jeff
On 1/10/23 21:20, Takayuki 'January June' Suwa via Gcc-patches wrote: > Currently, cond_move_process_if_block() does the conversion without > balancing the cost of the converted sequence with the original one, but > this should be checked by calling targetm.noce_conversion_profitable_p(). > > Doing so allows us to provide a way based on the target-specific cost > estimate, to prevent unwanted size growth due to excessive conditional > moves on optimizing for size. > > On optimizing for speed, default_noce_conversion_profitable_p() allows > plenty of headroom, so this patch has little impact. > > Also, if the target-specific cost estimate is accurate or allows for > margins, the impact should be similarly small. > > gcc/ChangeLog: > > * ifcvt.cc (cond_move_process_if_block): > Consider the result of targetm.noce_conversion_profitable_p() > when replacing the original sequence with the converted one. THanks. I pushed this to the trunk. Jeff
diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc index 008796838f7..a896e14bb3c 100644 --- a/gcc/ifcvt.cc +++ b/gcc/ifcvt.cc @@ -4350,7 +4350,7 @@ cond_move_process_if_block (struct noce_if_info *if_info) goto done; } seq = end_ifcvt_sequence (if_info); - if (!seq) + if (!seq || !targetm.noce_conversion_profitable_p (seq, if_info)) goto done; loc_insn = first_active_insn (then_bb);