Re: Revert PowerPC shrink-wrap support 3 of 3
On Thu, 10 Nov 2011, Hans-Peter Nilsson wrote: I think I need someone with appropriate write privileges to agree with that, and to also give 48h for someone to fix the problem. Sorry for not forthcoming on the second point. brgds, H-P PS. where is the policy written down, besides the mailing list archives? https://gcc.gnu.org/develop.html has had the following since 2003 or so: Patch Reversion If a patch is committed which introduces a regression on any target which the Steering Committee considers to be important and if: * the problem is reported to the original poster; * 48 hours pass without the original poster or any other party indicating that a fix will be forthcoming in the very near future; * two people with write privileges to the affected area of the compiler determine that the best course of action is to revert the patch; then they may revert the patch. (The list of important targets will be revised at the beginning of each release cycle, if necessary, and is part of the release criteria.) After the patch has been reverted, the poster may appeal the decision to the Steering Committee. Note that no distinction is made between patches which are themselves buggy and patches that expose latent bugs elsewhere in the compiler. This is there as part of the overall (release) methodology, though svnwrite.html would be at least as natural. Perhaps a reference there? Thoughts? Gerald PS: Yes, I am catching up with some older mails. ;-)
Re: Revert PowerPC shrink-wrap support 3 of 3
From: Hans-Peter Nilsson h...@axis.com Date: Thu, 10 Nov 2011 18:52:39 +0100 From: Hans-Peter Nilsson h...@axis.com Date: Thu, 10 Nov 2011 15:12:54 +0100 From: Bernd Schmidt ber...@codesourcery.com Date: Thu, 10 Nov 2011 14:29:04 +0100 HP, can you run full tests? Cross-test to cris-elf in progress. Thanks! Works, no regressions compared to before the breakage (r181187). Thanks! According to http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51051#c3 it fixes building for arm-unknown-linux-gnueabi too. AFAICT, your patch has got sufficiently testing now (on three targets to boot) to be considered safe to check in. Or is something amiss? (If it's the unchecked code quality you mentioned, that can be just as well dealt with having the tree in a working state; having the tree broken for some targets accomplishes nothing.) brgds, H-P
Re: Revert PowerPC shrink-wrap support 3 of 3
On Thu, Nov 10, 2011 at 11:38 AM, Hans-Peter Nilsson hans-peter.nils...@axis.com wrote: From: Hans-Peter Nilsson h...@axis.com Date: Wed, 9 Nov 2011 09:55:59 +0100 From: Alan Modra amo...@gmail.com Date: Tue, 1 Nov 2011 16:33:40 +0100 On Tue, Nov 01, 2011 at 12:57:22AM +1030, Alan Modra wrote: * function.c (bb_active_p): Delete. (dup_block_and_redirect, active_insn_between): New functions. (convert_jumps_to_returns, emit_return_for_exit): New functions, split out from.. (thread_prologue_and_epilogue_insns): ..here. Delete shadowing variables. Don't do prologue register clobber tests when shrink wrapping already failed. Delete all last_bb_active code. Instead compute tail block candidates for duplicating exit path. Remove these from antic set. Duplicate tails when reached from both blocks needing a prologue/epilogue and blocks not needing such. * ifcvt.c (dead_or_predicable): Test both flag_shrink_wrap and HAVE_simple_return. * bb-reorder.c (get_uncond_jump_length): Make global. * bb-reorder.h (get_uncond_jump_length): Declare. * cfgrtl.c (rtl_create_basic_block): Comment typo fix. (rtl_split_edge): Likewise. Warning fix. (rtl_duplicate_bb): New function. (rtl_cfg_hooks): Enable can_duplicate_block_p and duplicate_block. This (a revision in the range 181187:181189) broke build for cris-elf like so: See PR51051. Given that this also broke arm-linux-gnueabi, a primary platform, and Alan being absent until the 15th according to a message on IRC, I move to revert r181188. Is there a PR for the arm issue? I think I need someone with appropriate write privileges to agree with that, and to also give 48h for someone to fix the problem. Sorry for not forthcoming on the second point. Did you or somebody else try to look into the problem? To decide whether it's the best course of action it would be nice to know if it's a simple error in the patch that is easy to fix. brgds, H-P PS. where is the policy written down, besides the mailing list archives? http://gcc.gnu.org/develop.html
Re: Revert PowerPC shrink-wrap support 3 of 3
From: Richard Guenther richard.guent...@gmail.com Date: Thu, 10 Nov 2011 12:22:56 +0100 On Thu, Nov 10, 2011 at 11:38 AM, Hans-Peter Nilsson hans-peter.nils...@axis.com wrote: From: Hans-Peter Nilsson h...@axis.com Date: Wed, 9 Nov 2011 09:55:59 +0100 From: Alan Modra amo...@gmail.com Date: Tue, 1 Nov 2011 16:33:40 +0100 On Tue, Nov 01, 2011 at 12:57:22AM +1030, Alan Modra wrote: * function.c (bb_active_p): Delete. (dup_block_and_redirect, active_insn_between): New functions. (convert_jumps_to_returns, emit_return_for_exit): New functions, split out from.. (thread_prologue_and_epilogue_insns): ..here. Delete shadowing variables. Don't do prologue register clobber tests when shrink wrapping already failed. Delete all last_bb_active code. Instead compute tail block candidates for duplicating exit path. Remove these from antic set. Duplicate tails when reached from both blocks needing a prologue/epilogue and blocks not needing such. * ifcvt.c (dead_or_predicable): Test both flag_shrink_wrap and HAVE_simple_return. * bb-reorder.c (get_uncond_jump_length): Make global. * bb-reorder.h (get_uncond_jump_length): Declare. * cfgrtl.c (rtl_create_basic_block): Comment typo fix. (rtl_split_edge): Likewise. Warning fix. (rtl_duplicate_bb): New function. (rtl_cfg_hooks): Enable can_duplicate_block_p and duplicate_block. This (a revision in the range 181187:181189) broke build for cris-elf like so: See PR51051. Given that this also broke arm-linux-gnueabi, a primary platform, and Alan being absent until the 15th according to a message on IRC, I move to revert r181188. Is there a PR for the arm issue? It's covered by the same PR, see comment #1. I've now updated the target field. I think I need someone with appropriate write privileges to agree with that, and to also give 48h for someone to fix the problem. Sorry for not forthcoming on the second point. Did you or somebody else try to look into the problem? To decide whether it's the best course of action it would be nice to know if it's a simple error in the patch that is easy to fix. Nope, not really. Wouldn't FWIW, de jure matter, me not having write privileges to the affected area. Though, I had a quick look at the patch and nothing stood out except its intrusiveness, and it seems the patch wasn't tested on a !simple_return target (just powerpc-linux according to the replied-to message). brgds, H-P
Re: Revert PowerPC shrink-wrap support 3 of 3
On Thu, Nov 10, 2011 at 12:43 PM, Hans-Peter Nilsson hans-peter.nils...@axis.com wrote: From: Richard Guenther richard.guent...@gmail.com Date: Thu, 10 Nov 2011 12:22:56 +0100 On Thu, Nov 10, 2011 at 11:38 AM, Hans-Peter Nilsson hans-peter.nils...@axis.com wrote: From: Hans-Peter Nilsson h...@axis.com Date: Wed, 9 Nov 2011 09:55:59 +0100 From: Alan Modra amo...@gmail.com Date: Tue, 1 Nov 2011 16:33:40 +0100 On Tue, Nov 01, 2011 at 12:57:22AM +1030, Alan Modra wrote: * function.c (bb_active_p): Delete. (dup_block_and_redirect, active_insn_between): New functions. (convert_jumps_to_returns, emit_return_for_exit): New functions, split out from.. (thread_prologue_and_epilogue_insns): ..here. Delete shadowing variables. Don't do prologue register clobber tests when shrink wrapping already failed. Delete all last_bb_active code. Instead compute tail block candidates for duplicating exit path. Remove these from antic set. Duplicate tails when reached from both blocks needing a prologue/epilogue and blocks not needing such. * ifcvt.c (dead_or_predicable): Test both flag_shrink_wrap and HAVE_simple_return. * bb-reorder.c (get_uncond_jump_length): Make global. * bb-reorder.h (get_uncond_jump_length): Declare. * cfgrtl.c (rtl_create_basic_block): Comment typo fix. (rtl_split_edge): Likewise. Warning fix. (rtl_duplicate_bb): New function. (rtl_cfg_hooks): Enable can_duplicate_block_p and duplicate_block. This (a revision in the range 181187:181189) broke build for cris-elf like so: See PR51051. Given that this also broke arm-linux-gnueabi, a primary platform, and Alan being absent until the 15th according to a message on IRC, I move to revert r181188. Is there a PR for the arm issue? It's covered by the same PR, see comment #1. I've now updated the target field. I think I need someone with appropriate write privileges to agree with that, and to also give 48h for someone to fix the problem. Sorry for not forthcoming on the second point. Did you or somebody else try to look into the problem? To decide whether it's the best course of action it would be nice to know if it's a simple error in the patch that is easy to fix. Nope, not really. Wouldn't FWIW, de jure matter, me not having write privileges to the affected area. Though, I had a quick look at the patch and nothing stood out except its intrusiveness, and it seems the patch wasn't tested on a !simple_return target (just powerpc-linux according to the replied-to message). Fair enough. You can count me as one then, and I'll defer to Bernd to either provide a fix or ack the revert. Thanks, Richard. brgds, H-P
Re: Revert PowerPC shrink-wrap support 3 of 3
On 11/10/11 13:14, Richard Guenther wrote:
Fair enough. You can count me as one then, and I'll defer to Bernd
to either provide a fix or ack the revert.
I'm trying to track it down.
In 189r.outof_cfglayout, we have
(insn 31 33 35 3 (use (reg/i:SI 0 r0))
../../../../baseline-trunk/libstdc++-v3/libsupc++/new_opv.cc:34 -1
(nil))
;; Successors: EXIT [100.0%] (fallthru)
;; lr out 0 [r0] 11 [fp] 13 [sp] 14 [lr] 25 [sfp] 26 [afp]
;; live out 0 [r0] 11 [fp] 13 [sp] 25 [sfp] 26 [afp]
followed by a number of other basic blocks, so that looks wrong to me.
outof_cfglayout seems to assume that fallthrough edges to the exit block
are OK and don't need fixing up, and changing that seems nontrivial at
first glance.
The situation is first created during cfgcleanup in into_cfglayout. The
following patch makes the testcase compile by stopping the compiler from
moving the exit fallthru block around, but I've not checked whether it
has a negative effect on code quality. HP, can you run full tests?
Bernd
Index: ../baseline-trunk/gcc/cfgrtl.c
===
--- ../baseline-trunk/gcc/cfgrtl.c (revision 181252)
+++ ../baseline-trunk/gcc/cfgrtl.c (working copy)
@@ -2735,6 +2735,16 @@ cfg_layout_can_merge_blocks_p (basic_blo
if (BB_PARTITION (a) != BB_PARTITION (b))
return false;
+ /* If we would end up moving B's instructions, make sure it doesn't fall
+ through into the exit block, since we cannot recover from a fallthrough
+ edge into the exit block occurring in the middle of a function. */
+ if (NEXT_INSN (BB_END (a)) != BB_HEAD (b))
+{
+ edge e = find_fallthru_edge (b-succs);
+ if (e e-dest == EXIT_BLOCK_PTR)
+ return false;
+}
+
/* There must be exactly one edge in between the blocks. */
return (single_succ_p (a)
single_succ (a) == b
Re: Revert PowerPC shrink-wrap support 3 of 3
From: Hans-Peter Nilsson h...@axis.com Date: Thu, 10 Nov 2011 15:12:54 +0100 From: Bernd Schmidt ber...@codesourcery.com Date: Thu, 10 Nov 2011 14:29:04 +0100 HP, can you run full tests? Cross-test to cris-elf in progress. Thanks! Works, no regressions compared to before the breakage (r181187). Thanks! According to http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51051#c3 it fixes building for arm-unknown-linux-gnueabi too. brgds, H-P
Re: Revert PowerPC shrink-wrap support 3 of 3
On Thu, Nov 10, 2011 at 02:29:04PM +0100, Bernd Schmidt wrote: On 11/10/11 13:14, Richard Guenther wrote: Fair enough. You can count me as one then, and I'll defer to Bernd to either provide a fix or ack the revert. I'm trying to track it down. In 189r.outof_cfglayout, we have (insn 31 33 35 3 (use (reg/i:SI 0 r0)) ../../../../baseline-trunk/libstdc++-v3/libsupc++/new_opv.cc:34 -1 (nil)) ;; Successors: EXIT [100.0%] (fallthru) ;; lr out 0 [r0] 11 [fp] 13 [sp] 14 [lr] 25 [sfp] 26 [afp] ;; live out 0 [r0] 11 [fp] 13 [sp] 25 [sfp] 26 [afp] followed by a number of other basic blocks, so that looks wrong to me. outof_cfglayout seems to assume that fallthrough edges to the exit block are OK and don't need fixing up, and changing that seems nontrivial at first glance. The situation is first created during cfgcleanup in into_cfglayout. The following patch makes the testcase compile by stopping the compiler from moving the exit fallthru block around, but I've not checked whether it has a negative effect on code quality. HP, can you run full tests? FWIW, I did bootstrap and make check with/without this patch, and it introduces no regressions in the PowerPC, but I haven't look at the code generated. -- Michael Meissner, IBM 5 Technology Place Drive, M/S 2757, Westford, MA 01886-3141, USA meiss...@linux.vnet.ibm.com fax +1 (978) 399-6899
