From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23135 invoked by alias); 29 Nov 2012 15:51:11 -0000 Received: (qmail 23116 invoked by uid 22791); 29 Nov 2012 15:51:09 -0000 X-SWARE-Spam-Status: No, hits=-5.2 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 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; Thu, 29 Nov 2012 15:51:01 +0000 Received: by mail-lb0-f175.google.com with SMTP id gg13so8946579lbb.20 for ; Thu, 29 Nov 2012 07:50:59 -0800 (PST) Received: by 10.152.108.197 with SMTP id hm5mr22209449lab.45.1354204259249; Thu, 29 Nov 2012 07:50:59 -0800 (PST) MIME-Version: 1.0 Received: by 10.112.88.99 with HTTP; Thu, 29 Nov 2012 07:50:19 -0800 (PST) In-Reply-To: <20121129153852.GC10621@redhat.com> References: <20121126142843.GH17362@redhat.com> <1544820.Re9E01eJrW@polaris> <20121128182457.GB26585@redhat.com> <20121129153852.GC10621@redhat.com> From: Steven Bosscher Date: Thu, 29 Nov 2012 15:51:00 -0000 Message-ID: Subject: Re: [PATCH] Don't bypass blocks with multiple latch edges (PR middle-end/54838) To: Marek Polacek Cc: Richard Biener , Eric Botcazou , gcc-patches@gcc.gnu.org Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes 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 X-SW-Source: 2012-11/txt/msg02454.txt.bz2 On Thu, Nov 29, 2012 at 4:38 PM, Marek Polacek wrote: > On Thu, Nov 29, 2012 at 09:34:31AM +0100, Richard Biener wrote: >> Definitely not - that means to not preserve loops until after cprop. The goal >> is to preserve loops everywhere! > > Yikes, sorry, it wasn't clear to me what PROP_loops really does. Anyway, > I think I have a better fix now. The problem is just that when removing > BB 4 (which was a header), we have to zap ->header and ->latch. We > already have code for this: > > if (current_loops != NULL > && e->src->loop_father->latch == e->src) > { > /* ??? Now we are creating (or may create) a loop > with multiple entries. Simply mark it for > removal. Alternatively we could not do this > threading. */ > e->src->loop_father->header = NULL; > e->src->loop_father->latch = NULL; > } > > but the thing is that when there are multiple latch edges, then > ->latch is NULL. So we need to keep track of how many latch edges > the header has. Regtested/bootstrapped on x86_64, ok for trunk? > > Can I get rid of may_be_loop_header (and just use n_latch_edges > 0 > instead at that one place) in a followup? > > 2012-11-29 Marek Polacek <> > > PR middle-end/54838 > * cprop.c (bypass_block): Set header and latch to NULL when > BB has more than one latch edge. > (n_latches): New variable. You don't have to mention a new local variable in the ChangeLog. But FWIW, not all DFS back edges are latches. Maybe name it n_back_edges? > * gcc.dg/pr54838.c: New test. > > --- gcc/cprop.c.mp 2012-11-29 15:49:53.120524295 +0100 > +++ gcc/cprop.c 2012-11-29 15:50:01.421547832 +0100 > @@ -1499,6 +1499,7 @@ bypass_block (basic_block bb, rtx setcc, > int may_be_loop_header; > unsigned removed_p; > unsigned i; > + unsigned n_latch_edges; > edge_iterator ei; > > insn = (setcc != NULL) ? setcc : jump; > @@ -1510,13 +1511,12 @@ bypass_block (basic_block bb, rtx setcc, > if (note) > find_used_regs (&XEXP (note, 0), NULL); > > - may_be_loop_header = false; > + n_latch_edges = 0; > FOR_EACH_EDGE (e, ei, bb->preds) > if (e->flags & EDGE_DFS_BACK) > - { > - may_be_loop_header = true; > - break; > - } > + n_latch_edges++; > + > + may_be_loop_header = n_latch_edges > 0; > > change = 0; > for (ei = ei_start (bb->preds); (e = ei_safe_edge (ei)); ) > @@ -1605,7 +1605,8 @@ bypass_block (basic_block bb, rtx setcc, > && dest != EXIT_BLOCK_PTR) > { > if (current_loops != NULL > - && e->src->loop_father->latch == e->src) > + && (e->src->loop_father->latch == e->src > + || n_latch_edges > 1)) > { > /* ??? Now we are creating (or may create) a loop > with multiple entries. Simply mark it for It seems to me that this threading should just not happen. Creating loops with multiple entries is something to be avoided because most loop-based optimizations don't work on irreducible regions. So this affects all passes that run after CPROP, including unrolling, IRA, the scheduler, etc. There is already code that tries to avoid creating multi-entry loops: /* The irreducible loops created by redirecting of edges entering the loop from outside would decrease effectiveness of some of the following optimizations, so prevent this. */ if (may_be_loop_header && !(e->flags & EDGE_DFS_BACK)) { ei_next (&ei); continue; } Apparently your test case manages to slip through, and I wonder why. Ciao! Steven