This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix shrink-wrap bug with anticipating into loops (PR67778, PR68634)
- From: Richard Sandiford <richard dot sandiford at arm dot com>
- To: Segher Boessenkool <segher at kernel dot crashing dot org>
- Cc: Jakub Jelinek <jakub at redhat dot com>, gcc-patches at gcc dot gnu dot org, bschmidt at redhat dot com
- Date: Thu, 03 Dec 2015 15:20:04 +0000
- Subject: Re: [PATCH] Fix shrink-wrap bug with anticipating into loops (PR67778, PR68634)
- Authentication-results: sourceware.org; auth=none
- References: <ebfaeba0a9c98ddf1f8ea795efc2fd3ba082fdfe dot 1449080153 dot git dot segher at kernel dot crashing dot org> <20151202191905 dot GX5675 at tucnak dot redhat dot com> <20151202224547 dot GA823 at gate dot crashing dot org>
Segher Boessenkool <segher@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