Segher Boessenkool <seg...@kernel.crashing.org> writes: > On Wed, Dec 02, 2015 at 08:19:05PM +0100, Jakub Jelinek wrote: >> On Wed, Dec 02, 2015 at 06:21:47PM +0000, Segher Boessenkool wrote: >> > --- a/gcc/shrink-wrap.c >> > +++ b/gcc/shrink-wrap.c >> > @@ -752,7 +752,11 @@ try_shrink_wrapping (edge *entry_edge, bitmap_head >> > *bb_with, >> > >> > /* If we can move PRO back without having to duplicate more blocks, do >> > so. >> > We can move back to a block PRE if every path from PRE will >> > eventually >> > - need a prologue, that is, PRO is a post-dominator of PRE. */ >> > + need a prologue, that is, PRO is a post-dominator of PRE. We might >> > + need to duplicate PRE if there is any path from a successor of PRE >> > back >> > + to PRE, so don't allow that either (but self-loops are fine, as are >> > any >> > + other loops entirely dominated by PRE; this in general seems too >> > + expensive to check for, for such an uncommon case). */ >> >> So, what will happen if PRE self-loops? > > The prologue is put in a new block before the chosen one (as always). > >> It would be nice to have it covered by a testcase. > > If I knew how to prepare one, that stayed stable for more than about > two weeks, yes :-/ > >> > + bool ok = true; >> > + >> > + if (!can_get_prologue (pre, prologue_clobbered)) >> > + ok = false; >> > + >> > + FOR_EACH_EDGE (e, ei, pre->succs) >> > + if (e->dest != pre >> > + && dominated_by_p (CDI_POST_DOMINATORS, e->dest, pre)) >> > + ok = false; >> >> I wonder if it wouldn't be better to: >> >> if (!can_get_prologue (pre, prologue_clobbered)) >> ok = false; >> else >> FOR_EACH_EDGE (e, ei, pre->succs) >> if (e->dest != pre >> && dominated_by_p (CDI_POST_DOMINATORS, e->dest, pre)) >> { >> ok = false; >> break; >> } >> >> so that it doesn't walk or continue walking the edges if not needed. > > If the compiler is any good, neither does my code, right? :-) > > I think it is more important to have this code readable than a teeny > tiny bit faster. It is all linear (assuming dominator lookups are O(1)), > which isn't too hard to ascertain (yeah, famous last words).
Maybe the clearest thing is to split it out into a function that returns false as soon as it finds a reason why the transform is not OK. The "decent compiler" ought to inline that function. Thanks, Richard