From patchwork Sun Apr 28 12:26:04 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom de Vries X-Patchwork-Id: 240247 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]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "localhost", Issuer "www.qmailtoaster.com" (not verified)) by ozlabs.org (Postfix) with ESMTPS id D45042C00B7 for ; Sun, 28 Apr 2013 22:26:23 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=uz0VnWyxrEa/APD7V shLYv7bwQm6YkK8UckzQHEc5eyadYc75LI/RvN4vtfpY+GHjfWMUE/tzEgGxIrAR Y9KUalUzP+LTqCjyU/GgApeOuMBiquXRKDpxSpEjtdX6TkPsT0F7N90hzkNERkaD yifWhHZ2mJDWSjWgFI6otyRT+c= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=20Z748wsHbDx80OUqMrPFK+ G/lk=; b=JeUkC2nvVUe1tUKWAitNbVv0T0DnFrUc4BzJFNZ0amLZKLHMBUquKyd ekCu2IgVbM3cUBSXlSfWlMljaJ9UDWm6Cil5dRxhjSleEDON7H02MB6/cLl18yaL tbe/Vz2inyxEL16Hl3JkkEhTMZDou7Zr1mm5rCVyDG3oFptnPiso= Received: (qmail 28695 invoked by alias); 28 Apr 2013 12:26:16 -0000 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 Received: (qmail 28683 invoked by uid 89); 28 Apr 2013 12:26:15 -0000 X-Spam-SWARE-Status: No, score=-4.4 required=5.0 tests=AWL, BAYES_00, KHOP_RCVD_UNTRUST, KHOP_THREADED, RCVD_IN_HOSTKARMA_W, RCVD_IN_HOSTKARMA_WL autolearn=ham version=3.3.1 Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Sun, 28 Apr 2013 12:26:14 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1UWQgF-0001oh-Rf from Tom_deVries@mentor.com ; Sun, 28 Apr 2013 05:26:11 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by svr-orw-fem-01.mgc.mentorg.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Sun, 28 Apr 2013 05:26:11 -0700 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.2.247.3; Sun, 28 Apr 2013 13:26:09 +0100 Message-ID: <517D155C.3080509@mentor.com> Date: Sun, 28 Apr 2013 14:26:04 +0200 From: Tom de Vries User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 MIME-Version: 1.0 To: Richard Biener CC: , Jakub Jelinek Subject: Re: [PATCH] Preserve loops from CFG build until after RTL loop opts References: <517A8EE8.8010805@mentor.com> In-Reply-To: <517A8EE8.8010805@mentor.com> X-Virus-Found: No On 26/04/13 16:27, Tom de Vries wrote: > On 25/04/13 16:19, Richard Biener wrote: > >> and compared to the previous patch changed the tree-ssa-tailmerge.c >> part to deal with merging of loop latch and loop preheader (even >> if that's a really bad idea) to not regress gcc.dg/pr50763.c. >> Any suggestion on how to improve that part welcome. > So I think this is really a cornercase, and we should disregard it if that makes > things simpler. > > Rather than fixing up the loop structure, we could prevent tail-merge in these > cases. > > The current fix tests for current_loops == NULL, and I'm not sure that can still > happen there, given that we have PROP_loops. Richard, I've found that it happens in these g++ test-cases: g++.dg/ext/mv1.C g++.dg/ext/mv12.C g++.dg/ext/mv2.C g++.dg/ext/mv5.C g++.dg/torture/covariant-1.C g++.dg/torture/pr43068.C g++.dg/torture/pr47714.C This seems rare enough to just bail out of tail-merge in those cases. > It's not evident to me that the test bb2->loop_father->latch == bb2 is > sufficient. Before calling tail_merge_optimize, we call loop_optimizer_finalize > in which we assert that LOOPS_MAY_HAVE_MULTIPLE_LATCHES from there on, so in > theory we might miss some latches. > > But I guess that pre (having started out with simple latches) maintains simple > latches throughout, and that tail-merge does the same. I've added a comment related to this in the patch. Bootstrapped and reg-tested (ada inclusive) on x86_64. OK for trunk? Thanks, - Tom 2013-04-28 Tom de Vries * tree-ssa-tail-merge.c (find_same_succ_bb): Skip loop latch bbs. (replace_block_by): Don't set LOOPS_NEED_FIXUP. (tail_merge_optimize): Handle current_loops == NULL. * gcc.dg/pr50763.c: Update test. diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c index f2ab7444..5467ae5 100644 --- a/gcc/tree-ssa-tail-merge.c +++ b/gcc/tree-ssa-tail-merge.c @@ -689,7 +689,15 @@ find_same_succ_bb (basic_block bb, same_succ *same_p) edge_iterator ei; edge e; - if (bb == NULL) + if (bb == NULL + /* Be conservative with loop structure. It's not evident that this test + is sufficient. Before tail-merge, we've just called + loop_optimizer_finalize, and LOOPS_MAY_HAVE_MULTIPLE_LATCHES is now + set, so there's no guarantee that the loop->latch value is still valid. + But we assume that, since we've forced LOOPS_HAVE_SIMPLE_LATCHES at the + start of pre, we've kept that property intact throughout pre, and are + keeping it throughout tail-merge using this test. */ + || bb->loop_father->latch == bb) return; bitmap_set_bit (same->bbs, bb->index); FOR_EACH_EDGE (e, ei, bb->succs) @@ -1460,17 +1468,6 @@ replace_block_by (basic_block bb1, basic_block bb2) /* Mark the basic block as deleted. */ mark_basic_block_deleted (bb1); - /* ??? If we merge the loop preheader with the loop latch we are creating - additional entries into the loop, eventually rotating it. - Mark loops for fixup in this case. - ??? This is a completely unwanted transform and will wreck most - loops at this point - but with just not considering loop latches as - merge candidates we fail to commonize the two loops in gcc.dg/pr50763.c. - A better fix to avoid that regression is needed. */ - if (current_loops - && bb2->loop_father->latch == bb2) - loops_state_set (LOOPS_NEED_FIXUP); - /* Redirect the incoming edges of bb1 to bb2. */ for (i = EDGE_COUNT (bb1->preds); i > 0 ; --i) { @@ -1612,7 +1609,19 @@ tail_merge_optimize (unsigned int todo) int iteration_nr = 0; int max_iterations = PARAM_VALUE (PARAM_MAX_TAIL_MERGE_ITERATIONS); - if (!flag_tree_tail_merge || max_iterations == 0) + if (!flag_tree_tail_merge + || max_iterations == 0 + /* We try to be conservative with respect to loop structure, since: + - the cases where tail-merging could both affect loop structure and be + benificial are rare, + - it prevents us from having to fixup the loops using + loops_state_set (LOOPS_NEED_FIXUP), and + - keeping loop structure may allow us to simplify the pass. + In order to be conservative, we need loop information. In rare cases + (about 7 test-cases in the g++ testsuite) there is none (because + loop_optimizer_finalize has been called before tail-merge, and + PROP_loops is not set), so we bail out. */ + || current_loops == NULL) return 0; timevar_push (TV_TREE_TAIL_MERGE); diff --git a/gcc/testsuite/gcc.dg/pr50763.c b/gcc/testsuite/gcc.dg/pr50763.c index 60025e3..695b61c 100644 --- a/gcc/testsuite/gcc.dg/pr50763.c +++ b/gcc/testsuite/gcc.dg/pr50763.c @@ -12,5 +12,5 @@ foo (int c, int d) while (c == d); } -/* { dg-final { scan-tree-dump-times "== 33" 1 "pre"} } */ +/* { dg-final { scan-tree-dump-times "== 33" 2 "pre"} } */ /* { dg-final { cleanup-tree-dump "pre" } } */