From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 114355 invoked by alias); 14 Nov 2018 21:38:18 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 114338 invoked by uid 89); 14 Nov 2018 21:38:17 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 14 Nov 2018 21:38:16 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E68513154857; Wed, 14 Nov 2018 21:38:14 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-68.rdu2.redhat.com [10.10.112.68]) by smtp.corp.redhat.com (Postfix) with ESMTP id 60AF55E1CD; Wed, 14 Nov 2018 21:38:13 +0000 (UTC) Subject: Re: [PATCH 2/6] ifcvt: Allow constants operands in noce_convert_multiple_sets. To: Robin Dapp , gcc-patches@gcc.gnu.org Cc: krebbel@linux.ibm.com, iii@linux.ibm.com References: <20181114130752.5057-1-rdapp@linux.ibm.com> <20181114130752.5057-3-rdapp@linux.ibm.com> From: Jeff Law Openpgp: preference=signencrypt Message-ID: <004ab373-90cf-e1b4-9624-100a6fd9fec5@redhat.com> Date: Wed, 14 Nov 2018 21:38:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 MIME-Version: 1.0 In-Reply-To: <20181114130752.5057-3-rdapp@linux.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2018-11/txt/msg01332.txt.bz2 On 11/14/18 6:07 AM, Robin Dapp wrote: > This patch checks whether the current target supports conditional moves > with immediate then/else operands and allows noce_convert_multiple_sets > to deal with constants subsequently. > Also, minor refactoring is performed. > > -- > > gcc/ChangeLog: > > 2018-11-14 Robin Dapp > > * ifcvt.c (have_const_cmov): New function. > (noce_convert_multiple_sets): Allow constants if supported. > (bb_ok_for_noce_convert_multiple_sets): Likewise. > (check_cond_move_block): Refactor. > --- > gcc/ifcvt.c | 46 ++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 36 insertions(+), 10 deletions(-) > > diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c > index ddf077fa051..660bb46eb1c 100644 > --- a/gcc/ifcvt.c > +++ b/gcc/ifcvt.c > @@ -3077,6 +3077,27 @@ bb_valid_for_noce_process_p (basic_block test_bb, rtx cond, > return false; > } > > +/* Check if we have a movcc pattern that accepts constants as then/else > + operand (op 2/3). */ > +static bool > +have_const_cmov (machine_mode mode) > +{ > + enum insn_code icode; > + if ((icode = direct_optab_handler (movcc_optab, mode)) > + != CODE_FOR_nothing) > + { > + if (insn_data[(int) icode].operand[2].predicate > + && (insn_data[(int) icode].operand[2].predicate > + (const1_rtx, insn_data[(int) icode].operand[2].mode))) > + if (insn_data[(int) icode].operand[3].predicate > + && (insn_data[(int) icode].operand[3].predicate > + (const1_rtx, insn_data[(int) icode].operand[3].mode))) > + return true; > + } > + > + return false; > +} This may ultimately be too simplistic. There are targets where some constants are OK, but others may not be. By checking the predicate like this I think you can cause over-aggressive if-conversion if the target allows a range of integers in the expander's operand predicate, but allows a narrower range in the actual define_insn (presumably the expander loads them into a pseudo or somesuch in that case). We know that over-aggressive if-conversion into conditional moves hurts some targets. Ideally you'd create the actual insn with the constants you want to use and see if that's recognized as well as compute its cost. Is that just too painful at this point for some reason? > @@ -3689,7 +3717,7 @@ check_cond_move_block (basic_block bb, > { > rtx set, dest, src; > > - if (!NONDEBUG_INSN_P (insn) || JUMP_P (insn)) > + if (!active_insn_p (insn)) > continue; > set = single_set (insn); > if (!set) > @@ -3705,10 +3733,8 @@ check_cond_move_block (basic_block bb, > if (!CONSTANT_P (src) && !register_operand (src, VOIDmode)) > return FALSE; > > - if (side_effects_p (src) || side_effects_p (dest)) > - return FALSE; > - > - if (may_trap_p (src) || may_trap_p (dest)) > + /* Check for side effects and trapping. */ > + if (!noce_operand_ok (src) || !noce_operand_ok (dest)) > return FALSE; > > /* Don't try to handle this if the source register was These two hunks are probably OK as general cleanups. Note that noce_operand_ok is not strictly the same as checking side_effects_p and may_trap_p in the case of a MEM. But you've already filtered out MEMs before you get here. Jeff >