On Sun, 28 Apr 2013, Tom de Vries wrote:
> On 26/04/13 16:27, Tom de Vries wrote:
> > On 25/04/13 16:19, Richard Biener wrote:
> > <SNIP>
> >> 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.
> <SNIP>
> > 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?
+ 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;
A more complete test would be to use what the bb_loop_header_p predicate
does - skip latch _edges_. Not sure if that's easily possible in
the loop looking at succs
FOR_EACH_EDGE (e, ei, bb->succs)
{
int index = e->dest->index;
bitmap_set_bit (same->succs, index);
same_succ_edge_flags[index] = e->flags;
}
but we'd skip all edges for which
dominated_by_p (CDI_DOMINATORS, e->src, e->dest)
of course that's equal to skipping the whole basic-block if the
above is true.
I suppose the patch is ok as-is for now, but let's keep the above
in mind (I want to audit the whole bootstrap process for loops
that vanish and eventually re-appear, I just didn't get around
thinking about a proper way to efficiently instrument for that).
Thanks,
Richard.
