Patchwork CFG review needed for fix of "PowerPC shrink-wrap support 3 of 3"

login
register
mail settings
Submitter Alan Modra
Date Nov. 14, 2011, 9:56 p.m.
Message ID <20111114215648.GI14325@bubble.grove.modra.org>
Download mbox | patch
Permalink /patch/125617/
State New
Headers show

Comments

Alan Modra - Nov. 14, 2011, 9:56 p.m.
On Mon, Nov 14, 2011 at 07:48:03AM -1000, Richard Henderson wrote:
> On 11/14/2011 04:10 AM, Hans-Peter Nilsson wrote:
> > Looks like all we need is a positive review of
> > <http://gcc.gnu.org/ml/gcc-patches/2011-11/msg01409.html> and a
> > ChangeLog entry to unbreak three or more targets.
> > 
> > Someone with approval rights: pretty please?
> 
> That patch is ok.

Sorry for the bootstrap problems.  I believe the cause is my
extraction of convert_jumps_to_returns and emit_return_for_exit.  The
new HAVE_return code misses a single_succ_p test (covered by the
bitmap_bit_p (&bb_flags, ...) test in the HAVE_simple_return case).

I haven't really looked into what Bernd's fix does.  I know this one
fixes what I broke..

	* function.c (thread_prologue_and_epilogue_insns): Guard
	emitting return with single_succ_p test.
Richard Henderson - Nov. 15, 2011, 12:43 a.m.
On 11/14/2011 11:56 AM, Alan Modra wrote:
> On Mon, Nov 14, 2011 at 07:48:03AM -1000, Richard Henderson wrote:
>> On 11/14/2011 04:10 AM, Hans-Peter Nilsson wrote:
>>> Looks like all we need is a positive review of
>>> <http://gcc.gnu.org/ml/gcc-patches/2011-11/msg01409.html> and a
>>> ChangeLog entry to unbreak three or more targets.
>>>
>>> Someone with approval rights: pretty please?
>>
>> That patch is ok.
> 
> Sorry for the bootstrap problems.  I believe the cause is my
> extraction of convert_jumps_to_returns and emit_return_for_exit.  The
> new HAVE_return code misses a single_succ_p test (covered by the
> bitmap_bit_p (&bb_flags, ...) test in the HAVE_simple_return case).
> 
> I haven't really looked into what Bernd's fix does.  I know this one
> fixes what I broke..
> 
> 	* function.c (thread_prologue_and_epilogue_insns): Guard
> 	emitting return with single_succ_p test.


Hmm.  This looks plausible too.  

Bernd's patch made sure that cfglayout didn't do something impossible.
Of course, it's possible that his patch should merely be an assert,
and the correct fix goes here.

Thoughts?


r~
Bernd Schmidt - Nov. 15, 2011, 12:54 a.m.
On 11/15/11 01:43, Richard Henderson wrote:
> On 11/14/2011 11:56 AM, Alan Modra wrote:
>> 	* function.c (thread_prologue_and_epilogue_insns): Guard
>> 	emitting return with single_succ_p test.
> 
> Hmm.  This looks plausible too.  
> 
> Bernd's patch made sure that cfglayout didn't do something impossible.
> Of course, it's possible that his patch should merely be an assert,
> and the correct fix goes here.

I haven't really looked at Alan's patch; I'm thinking there were
probably two bugs - the one I found being latent before. Note that the
problematic CFG exists way before thread_prologue_and_epilogue even
runs, so an assert would still trigger.


Bernd
David Miller - Nov. 15, 2011, 2:33 a.m.
From: Bernd Schmidt <bernds@codesourcery.com>
Date: Tue, 15 Nov 2011 01:54:34 +0100

> On 11/15/11 01:43, Richard Henderson wrote:
>> On 11/14/2011 11:56 AM, Alan Modra wrote:
>>> 	* function.c (thread_prologue_and_epilogue_insns): Guard
>>> 	emitting return with single_succ_p test.
>> 
>> Hmm.  This looks plausible too.  
>> 
>> Bernd's patch made sure that cfglayout didn't do something impossible.
>> Of course, it's possible that his patch should merely be an assert,
>> and the correct fix goes here.
> 
> I haven't really looked at Alan's patch; I'm thinking there were
> probably two bugs - the one I found being latent before. Note that the
> problematic CFG exists way before thread_prologue_and_epilogue even
> runs, so an assert would still trigger.

I suspect that Alan's fix here will take care of the sparc ada bootstrap
regression reported, and regardless if it's correct it should be installed.
Richard Henderson - Nov. 15, 2011, 6:06 p.m.
On 11/14/2011 11:56 AM, Alan Modra wrote:
> 	* function.c (thread_prologue_and_epilogue_insns): Guard
> 	emitting return with single_succ_p test.

Ok.


r~

Patch

Index: gcc/function.c
===================================================================
--- gcc/function.c	(revision 181188)
+++ gcc/function.c	(working copy)
@@ -6230,7 +6230,8 @@  thread_prologue_and_epilogue_insns (void
 	      && !active_insn_between (BB_HEAD (last_bb), BB_END (last_bb)))
 	    convert_jumps_to_returns (last_bb, false, NULL);
 
-	  if (EDGE_COUNT (exit_fallthru_edge->src->preds) != 0)
+	  if (EDGE_COUNT (last_bb->preds) != 0
+	      && single_succ_p (last_bb))
 	    {
 	      last_bb = emit_return_for_exit (exit_fallthru_edge, false);
 	      epilogue_end = returnjump = BB_END (last_bb);