From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10479 invoked by alias); 19 Aug 2015 17:20:44 -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 10468 invoked by uid 89); 19 Aug 2015 17:20:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.6 required=5.0 tests=AWL,BAYES_50,SPF_PASS,UNSUBSCRIBE_BODY autolearn=no version=3.3.2 X-HELO: eu-smtp-delivery-143.mimecast.com Received: from eu-smtp-delivery-143.mimecast.com (HELO eu-smtp-delivery-143.mimecast.com) (207.82.80.143) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 19 Aug 2015 17:20:42 +0000 Received: from cam-owa1.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) by eu-smtp-1.mimecast.com with ESMTP id uk-mta-10-pjHBI4fER8-Rfmk3f1lb6g-1; Wed, 19 Aug 2015 18:20:36 +0100 Received: from [10.2.207.50] ([10.1.2.79]) by cam-owa1.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Wed, 19 Aug 2015 18:20:36 +0100 Message-ID: <55D4BAE4.9090403@arm.com> Date: Wed, 19 Aug 2015 17:20:00 -0000 From: Kyrill Tkachov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Jeff Law , Steven Bosscher CC: Bernhard Reutner-Fischer , GCC Patches Subject: Re: [PATCH][RTL-ifcvt] Make non-conditional execution if-conversion more aggressive References: <559FBB13.80009@arm.com> <55A388D3.10506@arm.com> <55A3C53F.7080706@arm.com> <55B150D4.5030909@redhat.com> <55B205F5.3080005@arm.com> <55B28737.5040403@redhat.com> <55B60525.1060404@arm.com> <55B657B2.70708@redhat.com> <55B65EFA.7010601@arm.com> <55B75601.7050808@arm.com> <55BBAFAC.5020608@redhat.com> <55CA2B56.4050804@redhat.com> <55CA2C36.6030001@arm.com> <55CB58CD.9040701@arm.com> <55D4B58F.3080609@redhat.com> In-Reply-To: <55D4B58F.3080609@redhat.com> X-MC-Unique: pjHBI4fER8-Rfmk3f1lb6g-1 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2015-08/txt/msg01121.txt.bz2 On 19/08/15 17:57, Jeff Law wrote: > On 08/12/2015 08:31 AM, Kyrill Tkachov wrote: >> 2015-08-10 Kyrylo Tkachov >> >> * ifcvt.c (struct noce_if_info): Add then_simple, else_simple, >> then_cost, else_cost fields. Change branch_cost field to unsigned >> int. >> (end_ifcvt_sequence): Call set_used_flags on each insn in the >> sequence. >> Include rtl-iter.h. >> (noce_simple_bbs): New function. >> (noce_try_move): Bail if basic blocks are not simple. >> (noce_try_store_flag): Likewise. >> (noce_try_store_flag_constants): Likewise. >> (noce_try_addcc): Likewise. >> (noce_try_store_flag_mask): Likewise. >> (noce_try_cmove): Likewise. >> (noce_try_minmax): Likewise. >> (noce_try_abs): Likewise. >> (noce_try_sign_mask): Likewise. >> (noce_try_bitop): Likewise. >> (bbs_ok_for_cmove_arith): New function. >> (noce_emit_all_but_last): Likewise. >> (noce_emit_insn): Likewise. >> (noce_emit_bb): Likewise. >> (noce_try_cmove_arith): Handle non-simple basic blocks. >> (insn_valid_noce_process_p): New function. >> (contains_mem_rtx_p): Likewise. >> (bb_valid_for_noce_process_p): Likewise. >> (noce_process_if_block): Allow non-simple basic blocks >> where appropriate. >> >> 2015-08-11 Kyrylo Tkachov >> >> * gcc.dg/ifcvt-1.c: New test. >> * gcc.dg/ifcvt-2.c: Likewise. >> * gcc.dg/ifcvt-3.c: Likewise. > Thanks for pinging -- I thought I'd already approved this a few days > ago! But I can't find it in my outbox... So clearly I didn't finish > the final review. > > > >> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c >> index 1f29646..c33fe24 100644 >> --- a/gcc/ifcvt.c >> +++ b/gcc/ifcvt.c >> @@ -1625,6 +1672,152 @@ noce_try_cmove (struct noce_if_info *if_info) >> return FALSE; >> } >> >> +/* Helper for bb_valid_for_noce_process_p. Validate that >> + the rtx insn INSN is a single set that does not set >> + the conditional register CC and is in general valid for >> + if-conversion. */ >> + >> +static bool >> +insn_valid_noce_process_p (rtx_insn *insn, rtx cc) >> +{ >> + if (!insn >> + || !NONJUMP_INSN_P (insn) >> + || (cc && set_of (cc, insn))) >> + return false; >> + >> + rtx sset =3D single_set (insn); >> + >> + /* Currently support only simple single sets in test_bb. */ >> + if (!sset >> + || !noce_operand_ok (SET_DEST (sset)) >> + || !noce_operand_ok (SET_SRC (sset))) >> + return false; >> + >> + return true; >> +} >> + >> + >> + /* Make sure this is a REG and not some instance >> + of ZERO_EXTRACT or SUBREG or other dangerous stuff. */ >> + if (!REG_P (SET_DEST (sset_b))) >> + { >> + BITMAP_FREE (bba_sets); >> + return false; >> + } > BTW, this is overly conservative. You're working with pseudos here, so > you can just treat a ZERO_EXTRACT or SUBREG as a read of the full > underlying register. If this comes up in practice you might consider > handling them as a follow-up patch. I don't think you need to handle > that case immediately though. I agree, from my testing and investigation this patch strictly increases the available opportunities, so being conservative here should not cause any regressions against existing behaviour. Making it more aggressive can be done as a follow up though, as you said, I'm not sure how frequently this comes up in practice. > > I also can't remember if we discussed what happens if blocks A & B write > to the same register, do we handle that situation correctly? Hmmm... The function bb_valid_for_noce_process_p that we call early on in noce_process_if_block makes sure that the only live reg out of each basic block is the final common destination ('x' in the noce_if_info struct definition). Since both basic blocks satisfy that check I suppose that means that even if A and B do write to the same intermediate pseudo that should not affect correctness since the written-to common register would have to be read within A and B (and nowhere outside A and B), which would cause it to fail the bbs_ok_for_cmove_arith check that checks that no regs written in A are read in B (and vice versa). > > That's the only issue left in my mind. If we're handling that case > correctly, then this is Ok for the trunk as-is. Else we'll need another > iteration. Does the above explanation look ok to you? If so, I'll be away for a week from Monday so I'd rather commit the patch when I get back so I can deal with any fallout... Thanks for the reviews! Kyrill > > Thanks, > Jeff > >