From patchwork Thu Nov 17 08:58:41 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom de Vries X-Patchwork-Id: 126159 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 DA3E5B71B9 for ; Thu, 17 Nov 2011 19:59:19 +1100 (EST) Received: (qmail 31850 invoked by alias); 17 Nov 2011 08:59:12 -0000 Received: (qmail 31838 invoked by uid 22791); 17 Nov 2011 08:59:09 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 17 Nov 2011 08:58:54 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1RQxo5-00061J-Fr from Tom_deVries@mentor.com ; Thu, 17 Nov 2011 00:58:53 -0800 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Thu, 17 Nov 2011 00:56:23 -0800 Received: from [127.0.0.1] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.1.289.1; Thu, 17 Nov 2011 08:58:51 +0000 Message-ID: <4EC4CCC1.4050802@mentor.com> Date: Thu, 17 Nov 2011 09:58:41 +0100 From: Tom de Vries User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.23) Gecko/20110922 Lightning/1.0b2 Thunderbird/3.1.15 MIME-Version: 1.0 To: Richard Henderson CC: Maxim Kuvyrkov , GCC Patches Subject: Re: [PR50764, PATCH] Fix for ICE in maybe_record_trace_start with -fsched2-use-superblocks References: <4EAC514A.2090607@mentor.com> <039B70DE-2742-4E7C-A265-4F3BB5CDA44C@codesourcery.com> In-Reply-To: <039B70DE-2742-4E7C-A265-4F3BB5CDA44C@codesourcery.com> 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 11/15/2011 10:07 PM, Maxim Kuvyrkov wrote: > On 30/10/2011, at 8:17 AM, Tom de Vries wrote: > >> Richard, >> >> I have a tentative fix for PR50764. > > Richard, > > Tom's patch is good (with the comments below addressed), and I would appreciate you validating my review with your formal approval. > Richard, Updated patch according to comments from Maxim. Added test-case. Bootstrapped and reg-tested on x86_64. Ok for trunk? Thanks, - Tom >> >> In the example from the test-case, -fsched2-use-superblocks moves an insn from >> block 4 to block 3. >> >> 2 >> bar >> | >> -------+----- >> / \ >> * * >> 5 *------------ 3 >> abort bar >> | >> | >> * >> 4 >> return >> >> >> The insn has a REG_CFA_DEF_CFA note and is frame-related. >> ... >> (insn/f 51 50 52 4 (set (reg:DI 39 r10) >> (mem/c:DI (plus:DI (reg/f:DI 6 bp) >> (const_int -8 [0xfffffffffffffff8])) [3 S8 A8])) pr50764.c:13 >> 62 {*movdi_internal_rex64} >> (expr_list:REG_CFA_DEF_CFA (reg:DI 39 r10) >> (nil))) >> ... >> >> This causes the assert in maybe_record_trace_start to trigger: >> ... >> /* We ought to have the same state incoming to a given trace no >> matter how we arrive at the trace. Anything else means we've >> got some kind of optimization error. */ >> gcc_checking_assert (cfi_row_equal_p (cur_row, ti->beg_row)); >> ... >> >> The assert does not occur with -fno-tree-tail-merge, but that is due to the >> following: >> - -fsched-use-superblocks does not handle dead labels explicitly >> - -freorder-blocks introduces a dead label, which is not removed until after >> sched2 >> - -ftree-tail-merge makes a difference in which block -freorder-blocks >> introduces the dead label. In the case of -ftree-tail-merge, the dead label >> is introduced at the start of block 3, and block 3 and 4 end up in the same >> ebb. In the case of -fno-tree-tail-merge, the dead label is introduced at the >> start of block 4, and block 3 and 4 don't end up in the same ebb. >> >> attached untested patch fixes PR50764 in a similar way as the patch for PR49994, >> which is also about an ICE in maybe_record_trace_start with >> -fsched2-use-superblocks. >> >> The patch for PR49994 makes sure frame-related instructions are not moved past >> the following jump. >> >> Attached patch makes sure frame-related instructions are not moved past the >> preceding jump. >> >> Is this the way to fix this PR? > > Tom, > > Thank you for good analysis, your patch is the right way to go. > > Scheduler should not move frame-related insns from either prologue or epilogue basic blocks. Currently sched-deps analysis handles the prologue case, and your patch fixes the epilogue case. The primary reason why we didn't hit the assert before is due to the fact that we do interblock scheduling after reload only on few architectures. With single-block scheduling after reload, which is what we do for most architectures, this issue cannot arise. > >> >> Index: gcc/sched-deps.c >> =================================================================== >> --- gcc/sched-deps.c (revision 180521) >> +++ gcc/sched-deps.c (working copy) >> @@ -2812,8 +2812,13 @@ sched_analyze_insn (struct deps_desc *de >> during prologue generation and avoid marking the frame pointer setup >> as frame-related at all. */ >> if (RTX_FRAME_RELATED_P (insn)) >> - deps->sched_before_next_jump >> - = alloc_INSN_LIST (insn, deps->sched_before_next_jump); >> + { >> + deps->sched_before_next_jump >> + = alloc_INSN_LIST (insn, deps->sched_before_next_jump); > > This code is rather obscure, so additional comments would be helpful. Please add something like "Make sure prologue INSN is scheduled before next jump." before the first statement; and add something like "Make sure epilogue INSN is not moved before preceding jumps." before the second statement. > >> + >> + if (deps->pending_jump_insns) >> + add_dependence (insn, XEXP (deps->pending_jump_insns, 0), REG_DEP_ANTI); > > Please use "add_dependence_list (insn, deps->pending_jump_insns, 1, REG_DEP_ANTI);" instead. We want INSN to depend upon all of pending jumps, not just one of them. The situation where pending_jump_insns has more than a single jump does not happen in current setup of scheduling runs (as sched-rgn does not do interblock scheduling after reload), but that may change in the future. > > OK upon formal approval from Richard or other reviewer. > > Thank you, > > -- > Maxim Kuvyrkov > CodeSourcery / Mentor Graphics > 2011-11-16 Tom de Vries PR rtl-optimization/50764 * (sched_analyze_insn): Make sure frame-related insns are not moved past preceding jump. * (gcc.dg/pr50764.c): New test. Index: gcc/sched-deps.c =================================================================== --- gcc/sched-deps.c (revision 181377) +++ gcc/sched-deps.c (working copy) @@ -2812,8 +2812,15 @@ sched_analyze_insn (struct deps_desc *de during prologue generation and avoid marking the frame pointer setup as frame-related at all. */ if (RTX_FRAME_RELATED_P (insn)) - deps->sched_before_next_jump - = alloc_INSN_LIST (insn, deps->sched_before_next_jump); + { + /* Make sure prologue insn is scheduled before next jump. */ + deps->sched_before_next_jump + = alloc_INSN_LIST (insn, deps->sched_before_next_jump); + + /* Make sure epilogue insn is scheduled after preceding jumps. */ + if (deps->pending_jump_insns) + add_dependence_list (insn, deps->pending_jump_insns, 1, REG_DEP_ANTI); + } if (code == COND_EXEC) { Index: gcc/testsuite/gcc.dg/pr50764.c =================================================================== --- /dev/null (new file) +++ gcc/testsuite/gcc.dg/pr50764.c (revision 0) @@ -0,0 +1,17 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fsched2-use-superblocks -ftree-tail-merge" } */ + +typedef int aligned __attribute__ ((aligned (64))); +extern void abort (void); + +int bar (void *p); + +void +foo (void) +{ + char *p = __builtin_alloca (13); + aligned i; + + if (bar (p) || bar (&i)) + abort (); +}