From patchwork Wed Oct 31 23:02:54 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Steven Bosscher X-Patchwork-Id: 196039 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 8B53C2C022F for ; Thu, 1 Nov 2012 10:03:36 +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=1352329416; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: MIME-Version:Received:In-Reply-To:References:From:Date: Message-ID:Subject:To:Cc:Content-Type:Mailing-List:Precedence: List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=8wvaj24nCrzpsTDhABpK0pTo4RA=; b=Ap7ltdOVnjRh87a vXIZTX/cg39zPAwqnK24HPgdbtNUUdKVmHPhjccltwirlaz2fVJT/pk37D2HjVpP XApi6jJmJULyedRP2PxeGy1eqEafOdNYaQpQZrivU8EyQWRzdTvNs1OAsww38H52 7qsZcBO0lByfW4Bi7IOhEPm8vU3Y= 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:Received:MIME-Version:Received:In-Reply-To:References:From:Date:Message-ID:Subject:To:Cc:Content-Type:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=RlDPA3w0XCfcIp5UQPlwGgLG2ayswJi8B5nkUeMn6MUWUuN6OT6srekNB/J4rz d2kVVP/L6k1jKGOPPkRCgLYwxbQybIAd3Q+pj4aRjJ/kTdyuSHmRnimyHMzIqPl7 nuv9kuYdRN07ZYo5a0WEJl8KAb2sEH11qav5TDxeG+ADk=; Received: (qmail 29344 invoked by alias); 31 Oct 2012 23:03:29 -0000 Received: (qmail 29321 invoked by uid 22791); 31 Oct 2012 23:03:27 -0000 X-SWARE-Spam-Status: No, hits=-5.1 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, KHOP_RCVD_TRUST, KHOP_THREADED, RCVD_IN_DNSWL_LOW, RCVD_IN_HOSTKARMA_YE, TW_CF, TW_GR X-Spam-Check-By: sourceware.org Received: from mail-lb0-f175.google.com (HELO mail-lb0-f175.google.com) (209.85.217.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 31 Oct 2012 23:03:17 +0000 Received: by mail-lb0-f175.google.com with SMTP id y2so1522132lbk.20 for ; Wed, 31 Oct 2012 16:03:15 -0700 (PDT) Received: by 10.112.50.106 with SMTP id b10mr15325999lbo.122.1351724595690; Wed, 31 Oct 2012 16:03:15 -0700 (PDT) MIME-Version: 1.0 Received: by 10.112.88.99 with HTTP; Wed, 31 Oct 2012 16:02:54 -0700 (PDT) In-Reply-To: References: <20121030052000.A4D0F61459@tjsboxrox.mtv.corp.google.com> From: Steven Bosscher Date: Thu, 1 Nov 2012 00:02:54 +0100 Message-ID: Subject: Re: [PATCH] Fix PR 53743 and other -freorder-blocks-and-partition failures (issue6823047) To: Teresa Johnson Cc: reply@codereview.appspotmail.com, David Li , "gcc-patches@gcc.gnu.org" 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 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: (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... Ciao! Steven 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,