| Message ID | CABu31nMAN=-q1_ThsJK2Ugd=Zh-vFi4C8OYcEeJhrsE7tinz6w@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Ping? On Thu, Dec 6, 2012 at 1:48 PM, Steven Bosscher wrote: > Hello, > > This patch has 3 parts: > > 1. Benign comment fixes. > > 2. Using DF_REF_REG_MEM_P idiom. Also benign. > > 3. Real bug fixes for cfglayout mode. > > For (3) the fixes are: > - Pointers to the unlinked parts of the insns chain are not cleared, > which results in complete RTL bodies being left not > garbage-collectable until the next function goes into cfglayout mode. > When compiling an artificial test case with two very large functions, > this patch reduces memory footprint by ~33%. > - Looking for BARRIERs as, well, barriers between basic blocks doesn't > work in cfglayout mode: the barriers are not there (they're in > BB_FOOTER, not in the insns chain). > > Bootstrapped&tested on powerpc64-unknown-linux-gnu. OK for trunk? > > Ciao! > Steven > > > > * bitmap.h: Fix "set_difference" references in comment. > * dse.c (dse_bitmap_obstack): Fix comment. > > * loop-invariant.c (record_use): Use DF_REF_REG_MEM_P instead of > looking at specific flags. > > * cfgrtl.c (rtl_verify_flow_info): Fix code style (indentation). > (fixup_reorder_chain): Set cfg_layout_function_header to NULL to > avoid dangling pointers into GC-allocated insns. Clear BB_HEADER, > BB_FOOTER, and cfg_layout_function_footer for the same reason. > Do not link new barriers for cfgrtl mode to cfglayout's BB_FOOTER. > * combine.c (combine_instructions): Fix end-of-block check to not > expect BARRIERs, which may not exist in cfglayout mode. > > Index: bitmap.h > =================================================================== > --- bitmap.h (revision 194247) > +++ bitmap.h (working copy) > @@ -81,7 +81,7 @@ along with GCC; see the file COPYING3. If not see > EXECUTE_IF_AND_IN_BITMAP > * set_union : bitmap_ior / bitmap_ior_into > * set_difference : bitmap_intersect_compl_p / > - bitmap_and_comp / bitmap_and_comp_into / > + bitmap_and_compl / bitmap_and_compl_into / > EXECUTE_IF_AND_COMPL_IN_BITMAP > * set_disjuction : bitmap_xor_comp / bitmap_xor_comp_into > * set_compare : bitmap_equal_p > Index: dse.c > =================================================================== > --- dse.c (revision 194247) > +++ dse.c (working copy) > @@ -204,7 +204,7 @@ along with GCC; see the file COPYING3. If not see > on the default obstack because these bitmaps can grow quite large > (~2GB for the small (!) test case of PR54146) and we'll hold on to > all that memory until the end of the compiler run. > - As a bonus, delete_tree_live_info can destroy all the bitmaps by just > + As a bonus, dse_step7 can destroy all the bitmaps by just > releasing the whole obstack. */ > static bitmap_obstack dse_bitmap_obstack; > > Index: loop-invariant.c > =================================================================== > --- loop-invariant.c (revision 194247) > +++ loop-invariant.c (working copy) > @@ -756,8 +756,7 @@ record_use (struct def *def, df_ref use) > > u->pos = DF_REF_REAL_LOC (use); > u->insn = DF_REF_INSN (use); > - u->addr_use_p = (DF_REF_TYPE (use) == DF_REF_REG_MEM_LOAD > - || DF_REF_TYPE (use) == DF_REF_REG_MEM_STORE); > + u->addr_use_p = DF_REF_REG_MEM_P (use); > u->next = def->uses; > def->uses = u; > def->n_uses++; > Index: cfgrtl.c > =================================================================== > --- cfgrtl.c (revision 194247) > +++ cfgrtl.c (working copy) > @@ -2361,13 +2361,13 @@ rtl_verify_flow_info (void) > break; > > /* And that the code outside of basic blocks has NULL bb field. */ > - if (!BARRIER_P (x) > - && BLOCK_FOR_INSN (x) != NULL) > - { > - error ("insn %d outside of basic blocks has non-NULL bb field", > - INSN_UID (x)); > - err = 1; > - } > + if (!BARRIER_P (x) > + && BLOCK_FOR_INSN (x) != NULL) > + { > + error ("insn %d outside of basic blocks has non-NULL bb field", > + INSN_UID (x)); > + err = 1; > + } > } > > if (!x) > @@ -3159,6 +3159,7 @@ fixup_reorder_chain (void) > insn = cfg_layout_function_header; > while (NEXT_INSN (insn)) > insn = NEXT_INSN (insn); > + cfg_layout_function_header = NULL_RTX; > } > > /* First do the bulk reordering -- rechain the blocks without regard to > @@ -3190,15 +3191,18 @@ fixup_reorder_chain (void) > while (NEXT_INSN (insn)) > insn = NEXT_INSN (insn); > } > + BB_HEADER (bb) = BB_FOOTER (bb) = NULL_RTX; > } > > NEXT_INSN (insn) = cfg_layout_function_footer; > if (cfg_layout_function_footer) > - PREV_INSN (cfg_layout_function_footer) = insn; > + { > + PREV_INSN (cfg_layout_function_footer) = insn; > + while (NEXT_INSN (insn)) > + insn = NEXT_INSN (insn); > + cfg_layout_function_footer = NULL_RTX; > + } > > - while (NEXT_INSN (insn)) > - insn = NEXT_INSN (insn); > - > set_last_insn (insn); > #ifdef ENABLE_CHECKING > verify_insn_chain (); > @@ -3242,7 +3246,7 @@ fixup_reorder_chain (void) > { > gcc_assert (!onlyjump_p (bb_end_insn) > || returnjump_p (bb_end_insn)); > - BB_FOOTER (bb) = emit_barrier_after (bb_end_insn); > + emit_barrier_after (bb_end_insn); > continue; > } > > Index: combine.c > =================================================================== > --- combine.c (revision 194247) > +++ combine.c (working copy) > @@ -1223,11 +1223,10 @@ combine_instructions (rtx f, unsigned int nregs) > if (NONDEBUG_INSN_P (insn)) > { > while (last_combined_insn > - && INSN_DELETED_P (last_combined_insn)) > + && INSN_DELETED_P (last_combined_insn) > + && last_combined_insn != BB_HEAD (this_basic_block)) > last_combined_insn = PREV_INSN (last_combined_insn); > if (last_combined_insn == NULL_RTX > - || BARRIER_P (last_combined_insn) > - || BLOCK_FOR_INSN (last_combined_insn) != this_basic_block > || DF_INSN_LUID (last_combined_insn) <= DF_INSN_LUID (insn)) > last_combined_insn = insn;
On Thu, Dec 06, 2012 at 01:48:31PM +0100, Steven Bosscher wrote: > Hello, > > This patch has 3 parts: > > 1. Benign comment fixes. > > 2. Using DF_REF_REG_MEM_P idiom. Also benign. > > 3. Real bug fixes for cfglayout mode. > > For (3) the fixes are: > - Pointers to the unlinked parts of the insns chain are not cleared, > which results in complete RTL bodies being left not > garbage-collectable until the next function goes into cfglayout mode. > When compiling an artificial test case with two very large functions, > this patch reduces memory footprint by ~33%. > - Looking for BARRIERs as, well, barriers between basic blocks doesn't > work in cfglayout mode: the barriers are not there (they're in > BB_FOOTER, not in the insns chain). > > Bootstrapped&tested on powerpc64-unknown-linux-gnu. OK for trunk? > > Ciao! > Steven > > > > * bitmap.h: Fix "set_difference" references in comment. > * dse.c (dse_bitmap_obstack): Fix comment. > > * loop-invariant.c (record_use): Use DF_REF_REG_MEM_P instead of > looking at specific flags. Dunno about the rest, but these parts look ok. Marek
On Thu, Dec 06, 2012 at 01:48:31PM +0100, Steven Bosscher wrote: > * bitmap.h: Fix "set_difference" references in comment. > * dse.c (dse_bitmap_obstack): Fix comment. This is ok. > > * loop-invariant.c (record_use): Use DF_REF_REG_MEM_P instead of > looking at specific flags. So is this. > * cfgrtl.c (rtl_verify_flow_info): Fix code style (indentation). This is undesirable, we generally don't fix indentation unless you touch the code (or very close surrounding code), yeah, we have lots of formatting issues (some people don't use tabs at all, etc.), but doing just formatting changes means making svn blame/git blame less useful. > (fixup_reorder_chain): Set cfg_layout_function_header to NULL to > avoid dangling pointers into GC-allocated insns. Clear BB_HEADER, > BB_FOOTER, and cfg_layout_function_footer for the same reason. > Do not link new barriers for cfgrtl mode to cfglayout's BB_FOOTER. > * combine.c (combine_instructions): Fix end-of-block check to not > expect BARRIERs, which may not exist in cfglayout mode. For the rest, I wonder if this really is stage3 material. Do you have a testcase that ICEs or is miscompiled without the changes? For the two GTY vars, perhaps we could temporarily clear them in clear_after_compilation instead. Jakub
On Wed, Dec 12, 2012 at 11:17 AM, Jakub Jelinek <jakub@redhat.com> wrote: >> * cfgrtl.c (rtl_verify_flow_info): Fix code style (indentation). > > This is undesirable, we generally don't fix indentation unless you touch > the code (or very close surrounding code), OK, I'll drop this then. >> (fixup_reorder_chain): Set cfg_layout_function_header to NULL to >> avoid dangling pointers into GC-allocated insns. Clear BB_HEADER, >> BB_FOOTER, and cfg_layout_function_footer for the same reason. >> Do not link new barriers for cfgrtl mode to cfglayout's BB_FOOTER. >> * combine.c (combine_instructions): Fix end-of-block check to not >> expect BARRIERs, which may not exist in cfglayout mode. > > For the rest, I wonder if this really is stage3 material. Do you have a > testcase that ICEs or is miscompiled without the changes? The fixup_reorder_chain patch fixes and ICE with hot-cold partitioning and GCAC checking. A BARRIER is removed somewhere along the line, but the BB_FOOTER pointer still points to it. I'm not sure how things go bad from there, but we end up with BB_FOOTER pointing to a collected barrier. The combine fix is a fix for a real bug. Your patch makes combine look across basic block boundaries and look for BARRIERs which can never be there. Technically this is a regression. Ciao! Steven
On Wed, Dec 12, 2012 at 11:29:28AM +0100, Steven Bosscher wrote: > >> (fixup_reorder_chain): Set cfg_layout_function_header to NULL to > >> avoid dangling pointers into GC-allocated insns. Clear BB_HEADER, > >> BB_FOOTER, and cfg_layout_function_footer for the same reason. > >> Do not link new barriers for cfgrtl mode to cfglayout's BB_FOOTER. > >> * combine.c (combine_instructions): Fix end-of-block check to not > >> expect BARRIERs, which may not exist in cfglayout mode. > > > > For the rest, I wonder if this really is stage3 material. Do you have a > > testcase that ICEs or is miscompiled without the changes? > > The fixup_reorder_chain patch fixes and ICE with hot-cold partitioning > and GCAC checking. A BARRIER is removed somewhere along the line, but > the BB_FOOTER pointer still points to it. I'm not sure how things go > bad from there, but we end up with BB_FOOTER pointing to a collected > barrier. Then a testcase should be added together with the patch and the change shouldn't be part of further unrelated changes. > The combine fix is a fix for a real bug. Your patch makes combine look > across basic block boundaries and look for BARRIERs which can never be > there. Technically this is a regression. I don't see how. If there is no barrier, but the prev note or insn doesn't belong to the current bb, then BLOCK_FOR_INSN (last_combined_insn) != this_basic_block will be true, and if there is no insn at all, last_combined_insn will be NULL. I view your combine.c change purely as a cleanup, thus IMHO stage 1 material. Jakub
On Wed, Dec 12, 2012 at 11:48 AM, Jakub Jelinek wrote: >> The fixup_reorder_chain patch fixes and ICE with hot-cold partitioning >> and GCAC checking. A BARRIER is removed somewhere along the line, but >> the BB_FOOTER pointer still points to it. I'm not sure how things go >> bad from there, but we end up with BB_FOOTER pointing to a collected >> barrier. > > Then a testcase should be added together with the patch and the change > shouldn't be part of further unrelated changes. You're kidding, right? A test case for a GCAC failure with profiling? I wouldn't even know how to write a test case for it :-) I'll resubmit for GCC 4.9. Instead of a test case, I have a verifier queued that asserts no BARRIERs appear in the insns stream when in cfglayout mode. (This verifier currently breaks hot-cold partitioning all over the place due to a bug in try_redirect_by_replacing_jump.) >> The combine fix is a fix for a real bug. Your patch makes combine look >> across basic block boundaries and look for BARRIERs which can never be >> there. Technically this is a regression. > > I don't see how. If there is no barrier, but the prev note or insn doesn't > belong to the current bb, then BLOCK_FOR_INSN (last_combined_insn) != this_basic_block > will be true, and if there is no insn at all, last_combined_insn will be > NULL. I view your combine.c change purely as a cleanup, thus IMHO stage 1 > material. Whatever, also fine. I assume you'll take care of this, given that you introduced this bug? Ciao! Steven
On Wed, Dec 12, 2012 at 1:17 PM, Steven Bosscher wrote: > I'll resubmit for GCC 4.9. Instead of a test case, I have a verifier > queued that asserts no BARRIERs appear in the insns stream when in > cfglayout mode. (This verifier currently breaks hot-cold partitioning > all over the place due to a bug in try_redirect_by_replacing_jump.) BTW, should there be a meta-bug for patches for GCC 4.9? Ciao! Steven
On Wed, Dec 12, 2012 at 01:41:49PM +0100, Steven Bosscher wrote: > On Wed, Dec 12, 2012 at 1:17 PM, Steven Bosscher wrote: > > I'll resubmit for GCC 4.9. Instead of a test case, I have a verifier > > queued that asserts no BARRIERs appear in the insns stream when in > > cfglayout mode. (This verifier currently breaks hot-cold partitioning > > all over the place due to a bug in try_redirect_by_replacing_jump.) > > BTW, should there be a meta-bug for patches for GCC 4.9? Yes, we had one for 4.8 too (and earlier releases too I think). Jakub
Index: bitmap.h =================================================================== --- bitmap.h (revision 194247) +++ bitmap.h (working copy) @@ -81,7 +81,7 @@ along with GCC; see the file COPYING3. If not see EXECUTE_IF_AND_IN_BITMAP * set_union : bitmap_ior / bitmap_ior_into * set_difference : bitmap_intersect_compl_p / - bitmap_and_comp / bitmap_and_comp_into / + bitmap_and_compl / bitmap_and_compl_into / EXECUTE_IF_AND_COMPL_IN_BITMAP * set_disjuction : bitmap_xor_comp / bitmap_xor_comp_into * set_compare : bitmap_equal_p Index: dse.c =================================================================== --- dse.c (revision 194247) +++ dse.c (working copy) @@ -204,7 +204,7 @@ along with GCC; see the file COPYING3. If not see on the default obstack because these bitmaps can grow quite large (~2GB for the small (!) test case of PR54146) and we'll hold on to all that memory until the end of the compiler run. - As a bonus, delete_tree_live_info can destroy all the bitmaps by just + As a bonus, dse_step7 can destroy all the bitmaps by just releasing the whole obstack. */ static bitmap_obstack dse_bitmap_obstack; Index: loop-invariant.c =================================================================== --- loop-invariant.c (revision 194247) +++ loop-invariant.c (working copy) @@ -756,8 +756,7 @@ record_use (struct def *def, df_ref use) u->pos = DF_REF_REAL_LOC (use); u->insn = DF_REF_INSN (use); - u->addr_use_p = (DF_REF_TYPE (use) == DF_REF_REG_MEM_LOAD - || DF_REF_TYPE (use) == DF_REF_REG_MEM_STORE); + u->addr_use_p = DF_REF_REG_MEM_P (use); u->next = def->uses; def->uses = u; def->n_uses++; Index: cfgrtl.c =================================================================== --- cfgrtl.c (revision 194247) +++ cfgrtl.c (working copy) @@ -2361,13 +2361,13 @@ rtl_verify_flow_info (void) break; /* And that the code outside of basic blocks has NULL bb field. */ - if (!BARRIER_P (x) - && BLOCK_FOR_INSN (x) != NULL) - { - error ("insn %d outside of basic blocks has non-NULL bb field", - INSN_UID (x)); - err = 1; - } + if (!BARRIER_P (x) + && BLOCK_FOR_INSN (x) != NULL) + { + error ("insn %d outside of basic blocks has non-NULL bb field", + INSN_UID (x)); + err = 1; + } } if (!x) @@ -3159,6 +3159,7 @@ fixup_reorder_chain (void) insn = cfg_layout_function_header; while (NEXT_INSN (insn)) insn = NEXT_INSN (insn); + cfg_layout_function_header = NULL_RTX; } /* First do the bulk reordering -- rechain the blocks without regard to @@ -3190,15 +3191,18 @@ fixup_reorder_chain (void) while (NEXT_INSN (insn)) insn = NEXT_INSN (insn); } + BB_HEADER (bb) = BB_FOOTER (bb) = NULL_RTX; } NEXT_INSN (insn) = cfg_layout_function_footer; if (cfg_layout_function_footer) - PREV_INSN (cfg_layout_function_footer) = insn; + { + PREV_INSN (cfg_layout_function_footer) = insn; + while (NEXT_INSN (insn)) + insn = NEXT_INSN (insn); + cfg_layout_function_footer = NULL_RTX; + } - while (NEXT_INSN (insn)) - insn = NEXT_INSN (insn); - set_last_insn (insn); #ifdef ENABLE_CHECKING verify_insn_chain (); @@ -3242,7 +3246,7 @@ fixup_reorder_chain (void) { gcc_assert (!onlyjump_p (bb_end_insn) || returnjump_p (bb_end_insn)); - BB_FOOTER (bb) = emit_barrier_after (bb_end_insn); + emit_barrier_after (bb_end_insn); continue; } Index: combine.c =================================================================== --- combine.c (revision 194247) +++ combine.c (working copy) @@ -1223,11 +1223,10 @@ combine_instructions (rtx f, unsigned int nregs) if (NONDEBUG_INSN_P (insn)) { while (last_combined_insn - && INSN_DELETED_P (last_combined_insn)) + && INSN_DELETED_P (last_combined_insn) + && last_combined_insn != BB_HEAD (this_basic_block)) last_combined_insn = PREV_INSN (last_combined_insn); if (last_combined_insn == NULL_RTX - || BARRIER_P (last_combined_insn) - || BLOCK_FOR_INSN (last_combined_insn) != this_basic_block || DF_INSN_LUID (last_combined_insn) <= DF_INSN_LUID (insn)) last_combined_insn = insn;
Hello, This patch has 3 parts: 1. Benign comment fixes. 2. Using DF_REF_REG_MEM_P idiom. Also benign. 3. Real bug fixes for cfglayout mode. For (3) the fixes are: - Pointers to the unlinked parts of the insns chain are not cleared, which results in complete RTL bodies being left not garbage-collectable until the next function goes into cfglayout mode. When compiling an artificial test case with two very large functions, this patch reduces memory footprint by ~33%. - Looking for BARRIERs as, well, barriers between basic blocks doesn't work in cfglayout mode: the barriers are not there (they're in BB_FOOTER, not in the insns chain). Bootstrapped&tested on powerpc64-unknown-linux-gnu. OK for trunk? Ciao! Steven * bitmap.h: Fix "set_difference" references in comment. * dse.c (dse_bitmap_obstack): Fix comment. * loop-invariant.c (record_use): Use DF_REF_REG_MEM_P instead of looking at specific flags. * cfgrtl.c (rtl_verify_flow_info): Fix code style (indentation). (fixup_reorder_chain): Set cfg_layout_function_header to NULL to avoid dangling pointers into GC-allocated insns. Clear BB_HEADER, BB_FOOTER, and cfg_layout_function_footer for the same reason. Do not link new barriers for cfgrtl mode to cfglayout's BB_FOOTER. * combine.c (combine_instructions): Fix end-of-block check to not expect BARRIERs, which may not exist in cfglayout mode.