From patchwork Thu Nov 1 17:19:07 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Teresa Johnson X-Patchwork-Id: 196300 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 4786A2C0314 for ; Fri, 2 Nov 2012 04:19:19 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1352395160; h=Comment: DomainKey-Signature:Received:Received:Received:Received: MIME-Version:Received:Received:In-Reply-To:References:Date: Message-ID:Subject:From:To:Cc:Content-Type: Content-Transfer-Encoding:Mailing-List:Precedence:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=SMcOUXWFAnyhMMGv6Jk2drHQZws=; b=LKFLVLwyq0BJuDP oebRIfbEsyA4Q/EhFlAmmv/8d2F0PWwO+Yfcr9la3LHNQK7vU4KYVHDE5/N/UWjm B4N8T7jn2VgdhzItnbctgGD0l1q8Ouh+nheYM97IrIiWmxG/kXKa4VxMwkSsZ0+H ZUFk5g88G2a5vMDfhBh9ZN+8JMio= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:X-Google-DKIM-Signature:MIME-Version:Received:Received:In-Reply-To:References:Date:Message-ID:Subject:From:To:Cc:Content-Type:Content-Transfer-Encoding:X-System-Of-Record:X-Gm-Message-State:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=B9i3veP/4FZ9JKPEILLCeFGu9e9Yu1FwnPTyPygHX6qhXd3EbKYUn8jYGoq/5u d33leUtht+Nkh/JRVE4fMXTD30nUtzSMzCGs7DxHiCQCnFrGCQnbv2CGZ9CMBnA/ y296KbhZF1RIzY2ZjHIx/YBTrQMn8/vCA5tuhzNNtTHlQ=; Received: (qmail 32261 invoked by alias); 1 Nov 2012 17:19:15 -0000 Received: (qmail 32252 invoked by uid 22791); 1 Nov 2012 17:19:14 -0000 X-SWARE-Spam-Status: No, hits=-5.9 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, KHOP_RCVD_TRUST, KHOP_THREADED, RCVD_IN_DNSWL_LOW, RCVD_IN_HOSTKARMA_YE, RP_MATCHES_RCVD, TW_CF, TW_GR X-Spam-Check-By: sourceware.org Received: from mail-pb0-f47.google.com (HELO mail-pb0-f47.google.com) (209.85.160.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 01 Nov 2012 17:19:08 +0000 Received: by mail-pb0-f47.google.com with SMTP id ro12so1809472pbb.20 for ; Thu, 01 Nov 2012 10:19:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding:x-system-of-record :x-gm-message-state; bh=z6saUOBGdIcf6WWzCap+qfUyCG+SMmf58gnleXaTdoY=; b=eyIl2eFvcEeqa10aExn/5ycSpknsBOvC/NCl7NE9MAmioD+jA79SZVHiKvjeIoEw6v DXDMldQgsheil5eknP3CtTBDJuIZurDS7zHh4oL/aYx9uye+5XxWqnOLS647YTqkWQPX +fxiYoJ5BVm44qMRiKqf6omQAv9btjjp30V69o/atnZXrcwH8HAZOlHysGgruNylzIKq w9qPwO+sTqBmEWm5vJaQoRkK337idUvPLZ9zLMBKI7QtSjBI2UwCq5apP4K7oxKouAga SEXU9YO1RbQEllwbRyFBvwmForAHQLgUpGJuSmnxxqvaWPjxyyNaAmpeqjX9F4KbIVCy aBQg== MIME-Version: 1.0 Received: by 10.68.237.6 with SMTP id uy6mr22481211pbc.147.1351790347652; Thu, 01 Nov 2012 10:19:07 -0700 (PDT) Received: by 10.68.189.38 with HTTP; Thu, 1 Nov 2012 10:19:07 -0700 (PDT) In-Reply-To: References: <20121030052000.A4D0F61459@tjsboxrox.mtv.corp.google.com> Date: Thu, 1 Nov 2012 10:19:07 -0700 Message-ID: Subject: Re: [PATCH] Fix PR 53743 and other -freorder-blocks-and-partition failures (issue6823047) From: Teresa Johnson To: Steven Bosscher Cc: reply@codereview.appspotmail.com, David Li , "gcc-patches@gcc.gnu.org" X-System-Of-Record: true X-Gm-Message-State: ALoCoQk/DqH6+/h/GajkEwiMgqKWmgDvxJ/1kAI2M7AcnbNNbk9Htx3OU8OcKoJg6/AZlpxP895HaDFEsnLlM4+FT/wPLIW7X2E3SzypCo+DjFZ6xod0Ch8xXpFXSckdPzE8Zh0fJ2PvMAbX6e//8SaeXWfk6Q7xwJ+N1lKHqtE9XLUOMBPy02qj7oqWYubCr0ft3jhm3062 X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org On Wed, Oct 31, 2012 at 4:02 PM, Steven Bosscher wrote: > On Tue, Oct 30, 2012 at 10:43 PM, Teresa Johnson wrote: >> Sure, I will give this a try after your verification patch tests >> complete. Does this mean that the patch you posted above to >> force_nonfallthru_and_redirect is no longer needed either? I'll see if >> I can avoid the need for some of my fixes, although I believe at least >> the function.c one will still be needed. I'll check. > > The force_nonfallthru change is still necessary, because > force_nonfallthru should be almost a no-op in cfglayout mode. The > whole concept of a fallthru edge doesn't exist in cfglayout mode: any > single_succ edge is a fallthru edge until the order of the basic > blocks has been determined and the insn chain is re-linked (cfglayout > mode originally was developed for bb-reorder, to move blocks around > more easily). So the correct patch would actually be: > > Index: cfgrtl.c > =================================================================== > --- cfgrtl.c (revision 193046) > +++ cfgrtl.c (working copy) > @@ -4547,7 +4547,7 @@ struct cfg_hooks cfg_layout_rtl_cfg_hooks = { > cfg_layout_split_edge, > rtl_make_forwarder_block, > NULL, /* tidy_fallthru_edge */ > - rtl_force_nonfallthru, > + NULL, /* force_nonfallthru */ > rtl_block_ends_with_call_p, > rtl_block_ends_with_condjump_p, > rtl_flow_call_edges_add, > > (Or better yet: Remove the force_nonfallthru and tidy_fallthru_edge > hooks, they are cfgrtl-only.) > > But obviously that won't work because > bb-reorder.c:fix_up_fall_thru_edges calls this hook while we're in > cfglayout mode. That is a bug. The call to force_nonfallthru results > in a "dangling" barrier: > > cfgrtl.c:1523 emit_barrier_after (BB_END (jump_block)); > > In cfglayout mode, barriers don't exist in the insns chain, and they > don't have to because every edge is a fallthru edge. If there are > barriers before cfglayout mode, they are either removed or linked in > the basic block footer, and fixup_reorder_chain restores or inserts > barriers where necessary to drop out of cfglayout mode. This > emit_barrier_after call hangs a barrier after BB_END but not in the > footer, and I'm pretty sure the result will be that the barrier is > lost in fixup_reorder_chain. See also emit_barrier_after_bb for how > inserting a barrier should be done in cfglayout mode. > > So in short, bbpart doesn't know what it wants to be: a cfgrtl or a > cfglayout pass. It doesn't work without cfglayout but it's doing > things that are only correct in the cfgrtl world and Very Wrong Indeed > in cfglayout-land. > > >> Regarding your earlier question about why we needed to add the >> barrier, I need to dig up the details again but essentially I found >> that the barriers were being added by bbpart, but bbro was reordering >> things and the block that ended up at the border between the hot and >> cold section didn't necessarily have a barrier on it because it was >> not previously at the region boundary. > > That doesn't sound right. bbpart doesn't actually re-order the basic > blocks, it only marks the blocks with the partition they will be > assigned to. Whatever ends up at the border between the two partitions > is not relevant: the hot section cannot end in a fall-through edge to > the cold section (verify_flow_info even checks for that, see "fallthru > edge crosses section boundary (bb %i)") so it must end in some > explicit jump. Such jumps are always followed by a barrier. The only > reason I can think of why there might be a missing barrier, is because > fixup_reorder_chain has a bug and forgets to insert the barrier in > some cases (and I suspect this may be the case for return patterns, or > the a.m. issue of a dropper barrier). > > I would like to work on debugging this, but it's hard without test cases... I'm working on trying to reproduce some of these failures in a test case I can share. So far, I have only been able to reproduce the failure reported in PR 53743 in spec2006 (456.hmmer/sre_math.c). Still working on getting a smaller/shareable test case for the other 2 issues. The failure in PR 53743 (assert in cfg_layout_merge_blocks) is what I had fixed with my original changes to cfgrtl.c. Need to understand why there is a reg crossing note between 2 bbs in the same partition. In the hmmer test case I also hit a failures in rtl_verify_flow_info and rtl_verify_flow_info_1: gcc -c -o sre_math.o -DSPEC_CPU -D NDEBUG -fprofile-use -freorder-blocks-and-partition -O2 sre_math.c sre_math.c: In function ‘Gammln’: sre_math.c:161:1: error: EDGE_CROSSING incorrectly set across same section } ^ sre_math.c:161:1: error: missing barrier after block 6 sre_math.c:161:1: internal compiler error: verify_flow_info failed This was due to some code in thread_prologue_and_epilogue_insns that duplicated tail blocks: if (e) { copy_bb = create_basic_block (NEXT_INSN (BB_END (e->src)), NULL_RTX, e->src); BB_COPY_PARTITION (copy_bb, e->src); } In this case e->src (bb 6) was in the cold section and e->dest was in the hot section, and e->src ended with a REG_CROSSING_JUMP followed by a barrier. The new copy_bb got put into the cold section by the copy partition above, leading to the first error. And because the create_basic_block call inserted the new copy_bb before NEXT_INSN (BB_END (e->src)), which in this case was the barrier, we ended up without the barrier after the crossing edge. I fixed this by making the following change: else { But I am not sure this is really correct in all cases - for example, what if another hot bb that also didn't require a prologue branched into the new cloned tail sequence, which is now cold? E.g. dup_block_and_redirect will redirect all predecessors that don't need a prologue to the new copy. I'm going to see if I can get the other 2 failures I had found to trigger on spec or a smaller test case. Teresa > > Ciao! > Steven --- Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 --- function.c (revision 192692) +++ function.c (working copy) @@ -6249,9 +6249,18 @@ thread_prologue_and_epilogue_insns (void) break; if (e) { + rtx note; copy_bb = create_basic_block (NEXT_INSN (BB_END (e->src)), NULL_RTX, e->src); BB_COPY_PARTITION (copy_bb, e->src); + /* Remove the region crossing note from jump at end of + e->src if it exists. */ + note = find_reg_note (BB_END (e->src), REG_CROSSING_JUMP, NULL_RTX); + if (note) + /* There would also have been a barrier after e->src, that + is now after copy_bb, but that shouldn't be a + problem?. */ + remove_note (BB_END (e->src), note); }