Message ID | CABu31nNHQhd-3_yJ-=yOzZ7v7ky1htPdVJFSoZbHQSpEzTmTQQ@mail.gmail.com |
---|---|
State | New |
Headers | show |
On Tue, Oct 30, 2012 at 9:26 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > Hello, > > Hot/cold partitioning is apparently a hot topic all of a sudden, which > is a good thing of course, because it's in need of some TLC. > > The attached patch adds another check the RTL cfg checking > (verify_flow_info) for the partitioning: A hot block can never be > dominated by a cold block (because the dominated block must also be > cold). This trips in PR55121. > > I haven't tested this with any profiling tests, but it's bound to > break things. From my POV, whatever gets broken by this patch was > already broken to begin with :-) If you're in CC, it's because I > hope you can help test this patch. I will try testing your patch on top of mine with our fdo benchmarks. For the others on the cc list, you may need to include my patch as well for testing. Without it, -freorder-blocks-and-partition was DOA for me. For my patch, see http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02692.html Teresa > > Downside of this patch is that I need dominance info. If it's not > available, I compute and free it. I'm not sure if this works if > dominance info status is DOM_NO_FAST_QUERY, and I don't want to > recompute in this case because IMHO a verifier should be a no-op from > the POV of the rest of the compiler, and updating dominators would > make this patch a not-a-no-op :-) > > Thoughts/comments? > > Ciao! > Steven > > * cfgrtl.c (rtl_verify_flow_info_1): Verify that blocks in the > hot partition are not dominated by blocks in the cold partition. > > Index: cfgrtl.c > =================================================================== > --- cfgrtl.c (revision 191819) > +++ cfgrtl.c (working copy) > @@ -2033,6 +2033,7 @@ rtl_verify_flow_info_1 (void) > rtx x; > int err = 0; > basic_block bb; > + bool have_partitions = false; > > /* Check the general integrity of the basic blocks. */ > FOR_EACH_BB_REVERSE (bb) > @@ -2145,6 +2146,8 @@ rtl_verify_flow_info_1 (void) > n_eh++; > else if (e->flags & EDGE_ABNORMAL) > n_abnormal++; > + > + have_partitions |= is_crossing; > } > > if (n_eh && !find_reg_note (BB_END (bb), REG_EH_REGION, NULL_RTX)) > @@ -2263,6 +2268,40 @@ rtl_verify_flow_info_1 (void) > } > } > > + /* If there are partitions, do a sanity check on them: A basic block in > + a cold partition cannot dominate a basic block in a hot partition. */ > + VEC (basic_block, heap) *bbs_in_cold_partition = NULL; > + if (have_partitions && !err) > + FOR_EACH_BB (bb) > + if ((BB_PARTITION (bb) == BB_COLD_PARTITION)) > + VEC_safe_push (basic_block, heap, bbs_in_cold_partition, bb); > + if (! VEC_empty (basic_block, bbs_in_cold_partition)) > + { > + bool dom_calculated_here = !dom_info_available_p (CDI_DOMINATORS); > + basic_block son; > + > + if (dom_calculated_here) > + calculate_dominance_info (CDI_DOMINATORS); > + > + while (! VEC_empty (basic_block, bbs_in_cold_partition)) > + { > + bb = VEC_pop (basic_block, bbs_in_cold_partition); > + if ((BB_PARTITION (bb) != BB_COLD_PARTITION)) > + { > + error ("non-cold basic block %d dominated " > + "by a block in the cold partition", bb->index); > + err = 1; > + } > + for (son = first_dom_son (CDI_DOMINATORS, bb); > + son; > + son = next_dom_son (CDI_DOMINATORS, son)) > + VEC_safe_push (basic_block, heap, bbs_in_cold_partition, son); > + } > + > + if (dom_calculated_here) > + free_dominance_info (CDI_DOMINATORS); > + } > + > /* Clean up. */ > return err; > }
On Tue, Oct 30, 2012 at 5:59 PM, Teresa Johnson wrote: > I will try testing your patch on top of mine with our fdo benchmarks. Thanks. But you should expect a lot of errors, hopefully you can make something out of it for Bugzilla. > For the others on the cc list, you may need to include my patch as > well for testing. Without it, -freorder-blocks-and-partition was DOA > for me. For my patch, see > http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02692.html Ah, the fate of a pass that isn't enabled by default. From what I can tell, looking at this code the past few hours, it's hopelessly broken at the moment: * It doesn't work with cfglayout mode, even though it is in between pass_into_cfg_layout_mode and pass_outof_cfg_layout_mode. Coming out of cfglayout mode * Coming out of cfglayout mode, fixup_reorder_chain adds REG_CROSSING_JUMP -- but so does force_nonfallthru_and_redirect called just before it. So we end up with >1 such note, or with notes on jumps that shouldn't have them. * The scheduler doesn't respect the partitioning at all. remove_notes happily removes the section split note. * etc. This pass may need some major work to be in good order again. Ciao! Steven
On Tue, Oct 30, 2012 at 12:25 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > On Tue, Oct 30, 2012 at 5:59 PM, Teresa Johnson wrote: >> I will try testing your patch on top of mine with our fdo benchmarks. > > Thanks. But you should expect a lot of errors, hopefully you can make > something out of it for Bugzilla. > >> For the others on the cc list, you may need to include my patch as >> well for testing. Without it, -freorder-blocks-and-partition was DOA >> for me. For my patch, see >> http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02692.html > > Ah, the fate of a pass that isn't enabled by default. I see only 5 test cases under tree-prof for function splitting. More test cases are needed. David > From what I can > tell, looking at this code the past few hours, it's hopelessly broken > at the moment: > > * It doesn't work with cfglayout mode, even though it is in between > pass_into_cfg_layout_mode and pass_outof_cfg_layout_mode. Coming out > of cfglayout mode > > * Coming out of cfglayout mode, fixup_reorder_chain adds > REG_CROSSING_JUMP -- but so does force_nonfallthru_and_redirect called > just before it. So we end up with >1 such note, or with notes on jumps > that shouldn't have them. > > * The scheduler doesn't respect the partitioning at all. remove_notes > happily removes the section split note. > > * etc. > > > This pass may need some major work to be in good order again. > > Ciao! > Steven
On Tue, Oct 30, 2012 at 12:33 PM, Xinliang David Li <davidxl@google.com> wrote: > On Tue, Oct 30, 2012 at 12:25 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: >> On Tue, Oct 30, 2012 at 5:59 PM, Teresa Johnson wrote: >>> I will try testing your patch on top of mine with our fdo benchmarks. >> >> Thanks. But you should expect a lot of errors, hopefully you can make >> something out of it for Bugzilla. I didn't hit it, but I see it looks like I need to enable checking to get to this verification routine. Just rebuilt with checking enabled and redoing my builds. >> >>> For the others on the cc list, you may need to include my patch as >>> well for testing. Without it, -freorder-blocks-and-partition was DOA >>> for me. For my patch, see >>> http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02692.html >> >> Ah, the fate of a pass that isn't enabled by default. > > I see only 5 test cases under tree-prof for function splitting. More > test cases are needed. In an attempt to provoke the failures I was seeing on a smaller test case, I tried enabling -freorder-blocks-and-partition by default in my common.opt file and running through the regression test suite (without my fixes). But I didn't hit any of the failures. Perhaps there aren't many profile-use tests. Teresa > > David > > >> From what I can >> tell, looking at this code the past few hours, it's hopelessly broken >> at the moment: >> >> * It doesn't work with cfglayout mode, even though it is in between >> pass_into_cfg_layout_mode and pass_outof_cfg_layout_mode. Coming out >> of cfglayout mode >> >> * Coming out of cfglayout mode, fixup_reorder_chain adds >> REG_CROSSING_JUMP -- but so does force_nonfallthru_and_redirect called >> just before it. So we end up with >1 such note, or with notes on jumps >> that shouldn't have them. >> >> * The scheduler doesn't respect the partitioning at all. remove_notes >> happily removes the section split note. >> >> * etc. >> >> >> This pass may need some major work to be in good order again. >> >> Ciao! >> Steven -- Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
There are not many -- see gcc/testsuite/gcc.dg/tree-prof directory. Under that, 5 cases explicitly set the option. David On Tue, Oct 30, 2012 at 2:31 PM, Teresa Johnson <tejohnson@google.com> wrote: > On Tue, Oct 30, 2012 at 12:33 PM, Xinliang David Li <davidxl@google.com> wrote: >> On Tue, Oct 30, 2012 at 12:25 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: >>> On Tue, Oct 30, 2012 at 5:59 PM, Teresa Johnson wrote: >>>> I will try testing your patch on top of mine with our fdo benchmarks. >>> >>> Thanks. But you should expect a lot of errors, hopefully you can make >>> something out of it for Bugzilla. > > I didn't hit it, but I see it looks like I need to enable checking to > get to this verification routine. Just rebuilt with checking enabled > and redoing my builds. > >>> >>>> For the others on the cc list, you may need to include my patch as >>>> well for testing. Without it, -freorder-blocks-and-partition was DOA >>>> for me. For my patch, see >>>> http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02692.html >>> >>> Ah, the fate of a pass that isn't enabled by default. >> >> I see only 5 test cases under tree-prof for function splitting. More >> test cases are needed. > > In an attempt to provoke the failures I was seeing on a smaller test > case, I tried enabling -freorder-blocks-and-partition by default in my > common.opt file and running through the regression test suite (without > my fixes). But I didn't hit any of the failures. Perhaps there aren't > many profile-use tests. > > Teresa > > >> >> David >> >> >>> From what I can >>> tell, looking at this code the past few hours, it's hopelessly broken >>> at the moment: >>> >>> * It doesn't work with cfglayout mode, even though it is in between >>> pass_into_cfg_layout_mode and pass_outof_cfg_layout_mode. Coming out >>> of cfglayout mode >>> >>> * Coming out of cfglayout mode, fixup_reorder_chain adds >>> REG_CROSSING_JUMP -- but so does force_nonfallthru_and_redirect called >>> just before it. So we end up with >1 such note, or with notes on jumps >>> that shouldn't have them. >>> >>> * The scheduler doesn't respect the partitioning at all. remove_notes >>> happily removes the section split note. >>> >>> * etc. >>> >>> >>> This pass may need some major work to be in good order again. >>> >>> Ciao! >>> Steven > > > > -- > Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
On Tue, Oct 30, 2012 at 10:31 PM, Teresa Johnson wrote: > In an attempt to provoke the failures I was seeing on a smaller test > case, I tried enabling -freorder-blocks-and-partition by default in my > common.opt file and running through the regression test suite (without > my fixes). But I didn't hit any of the failures. Perhaps there aren't > many profile-use tests. Not many, here are all of them: $ egrep -r --exclude-dir=".svn" "profile-use|fbranch-prob" * | egrep -v "tree-prof|\.exp|Chan" gcc.dg/pr51957-1.c:/* { dg-options "-O2 -g -fprofile-use" } */ gcc.dg/pr32773.c:/* { dg-options "-O -fprofile-use" } */ gcc.dg/pr32773.c:/* { dg-options "-O -m4 -fprofile-use" { target sh-*-* } } */ gcc.dg/pr40209.c:/* { dg-options "-O2 -fprofile-use" } */ gcc.dg/pr26570.c:/* { dg-options "-O2 -fprofile-generate -fprofile-use" } */ g++.dg/tree-ssa/dom-invalid.C:// { dg-options "-Wall -fno-exceptions -O2 -fprofile-use -fno-rtti" } $ ls g*/tree-prof/* gcc.dg/tree-prof/bb-reorg.c gcc.dg/tree-prof/pr49299-2.c gcc.dg/tree-prof/unroll-1.c gcc.dg/tree-prof/wcoverage-mismatch.c gcc.dg/tree-prof/ic-misattribution-1a.c gcc.dg/tree-prof/pr50907.c gcc.dg/tree-prof/update-cunroll-2.c g++.dg/tree-prof/indir-call-prof-2.C gcc.dg/tree-prof/ic-misattribution-1.c gcc.dg/tree-prof/pr52027.c gcc.dg/tree-prof/update-loopch.c g++.dg/tree-prof/indir-call-prof.C gcc.dg/tree-prof/indir-call-prof.c gcc.dg/tree-prof/pr52150.c gcc.dg/tree-prof/update-tailcall.c g++.dg/tree-prof/inline_mismatch_args.C gcc.dg/tree-prof/inliner-1.c gcc.dg/tree-prof/prof-robust-1.c gcc.dg/tree-prof/val-prof-1.c g++.dg/tree-prof/partition1.C gcc.dg/tree-prof/peel-1.c gcc.dg/tree-prof/stringop-1.c gcc.dg/tree-prof/val-prof-2.c g++.dg/tree-prof/partition2.C gcc.dg/tree-prof/pr34999.c gcc.dg/tree-prof/stringop-2.c gcc.dg/tree-prof/val-prof-3.c g++.dg/tree-prof/partition3.C gcc.dg/tree-prof/pr44777.c gcc.dg/tree-prof/switch-case-1.c gcc.dg/tree-prof/val-prof-4.c g++.dg/tree-prof/pr51719.C gcc.dg/tree-prof/pr45354.c gcc.dg/tree-prof/switch-case-2.c gcc.dg/tree-prof/val-prof-5.c g++.dg/tree-prof/pr53460.C gcc.dg/tree-prof/pr47187.c gcc.dg/tree-prof/tracer-1.c gcc.dg/tree-prof/val-prof-6.c g++.dg/tree-prof/tree-prof.exp gcc.dg/tree-prof/pr49299-1.c gcc.dg/tree-prof/tree-prof.exp gcc.dg/tree-prof/val-prof-7.c
On Tue, Oct 30, 2012 at 2:31 PM, Teresa Johnson <tejohnson@google.com> wrote: > On Tue, Oct 30, 2012 at 12:33 PM, Xinliang David Li <davidxl@google.com> wrote: >> On Tue, Oct 30, 2012 at 12:25 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: >>> On Tue, Oct 30, 2012 at 5:59 PM, Teresa Johnson wrote: >>>> I will try testing your patch on top of mine with our fdo benchmarks. >>> >>> Thanks. But you should expect a lot of errors, hopefully you can make >>> something out of it for Bugzilla. > > I didn't hit it, but I see it looks like I need to enable checking to > get to this verification routine. Just rebuilt with checking enabled > and redoing my builds. Well it fired all over the place. The one case I looked at was due to insane bb counts introduced by loop unrolling. Before unrolling, the loop preheader had a count of 2 and the loop entry block a count of 32K. After unrolling by 1, there is a new loop entry block that is created and although the edge to it has a count of 2, the new BB has a count of 0. When we do bb partitioning, this new preheader bb is then placed in the cold section, while the loop entry bb is in the hot section, leading to the error message. While it would be good to fix insanities like this (I can file a separate PR for the loop unroll issue unless I find an existing one), for now bb partitioning should probably just detect the insanity and place the dominating "cold" block in the hot section. This should help improve the performance under -freorder-blocks-and-partition. I can work on this unless you prefer to take a stab at it. I'm going to switch to testing your other patch though next (that moves the bbpart closer to bbro), and revisit the changes from my patch. Thanks, Teresa > >>> >>>> For the others on the cc list, you may need to include my patch as >>>> well for testing. Without it, -freorder-blocks-and-partition was DOA >>>> for me. For my patch, see >>>> http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02692.html >>> >>> Ah, the fate of a pass that isn't enabled by default. >> >> I see only 5 test cases under tree-prof for function splitting. More >> test cases are needed. > > In an attempt to provoke the failures I was seeing on a smaller test > case, I tried enabling -freorder-blocks-and-partition by default in my > common.opt file and running through the regression test suite (without > my fixes). But I didn't hit any of the failures. Perhaps there aren't > many profile-use tests. > > Teresa > > >> >> David >> >> >>> From what I can >>> tell, looking at this code the past few hours, it's hopelessly broken >>> at the moment: >>> >>> * It doesn't work with cfglayout mode, even though it is in between >>> pass_into_cfg_layout_mode and pass_outof_cfg_layout_mode. Coming out >>> of cfglayout mode >>> >>> * Coming out of cfglayout mode, fixup_reorder_chain adds >>> REG_CROSSING_JUMP -- but so does force_nonfallthru_and_redirect called >>> just before it. So we end up with >1 such note, or with notes on jumps >>> that shouldn't have them. >>> >>> * The scheduler doesn't respect the partitioning at all. remove_notes >>> happily removes the section split note. >>> >>> * etc. >>> >>> >>> This pass may need some major work to be in good order again. >>> >>> Ciao! >>> Steven > > > > -- > Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
On 30.10.2012 17:59, Teresa Johnson wrote: > On Tue, Oct 30, 2012 at 9:26 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: >> Hello, >> >> Hot/cold partitioning is apparently a hot topic all of a sudden, which >> is a good thing of course, because it's in need of some TLC. >> >> The attached patch adds another check the RTL cfg checking >> (verify_flow_info) for the partitioning: A hot block can never be >> dominated by a cold block (because the dominated block must also be >> cold). This trips in PR55121. >> >> I haven't tested this with any profiling tests, but it's bound to >> break things. From my POV, whatever gets broken by this patch was >> already broken to begin with :-) If you're in CC, it's because I >> hope you can help test this patch. > I will try testing your patch on top of mine with our fdo benchmarks. > For the others on the cc list, you may need to include my patch as > well for testing. Without it, -freorder-blocks-and-partition was DOA > for me. For my patch, see > http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02692.html > > Teresa > I have tried Steven's patch an indeed it reported numerous errors while compiling spec2k. I tried Teresa's patch too, but it changed nothing in my tests. The patches already posted by Matt are still necessary and Teresa's patch does not improve my tests. I am out of office at the moment, so it's a little bit inconvenient to investigate deeper the reasons for all the errors reported by Steven's patch. Anyway it looks like it's really needed :) I also noticed that some compilations failed with an ICE in calc_dfs_tree at dominance.c:395. Christophe.
On Wed, Oct 31, 2012 at 12:58 PM, Christophe Lyon <christophe.lyon@st.com> wrote: > On 30.10.2012 17:59, Teresa Johnson wrote: >> >> On Tue, Oct 30, 2012 at 9:26 AM, Steven Bosscher <stevenb.gcc@gmail.com> >> wrote: >>> >>> Hello, >>> >>> Hot/cold partitioning is apparently a hot topic all of a sudden, which >>> is a good thing of course, because it's in need of some TLC. >>> >>> The attached patch adds another check the RTL cfg checking >>> (verify_flow_info) for the partitioning: A hot block can never be >>> dominated by a cold block (because the dominated block must also be >>> cold). This trips in PR55121. >>> >>> I haven't tested this with any profiling tests, but it's bound to >>> break things. From my POV, whatever gets broken by this patch was >>> already broken to begin with :-) If you're in CC, it's because I >>> hope you can help test this patch. >> >> I will try testing your patch on top of mine with our fdo benchmarks. >> For the others on the cc list, you may need to include my patch as >> well for testing. Without it, -freorder-blocks-and-partition was DOA >> for me. For my patch, see >> http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02692.html >> >> Teresa >> > I have tried Steven's patch an indeed it reported numerous errors while > compiling spec2k. > > I tried Teresa's patch too, but it changed nothing in my tests. The patches > already posted by Matt are still necessary and Teresa's patch does not > improve my tests. With checking enabled I am seeing additional failures that my fixes are not addressing. Looking into those now. Can someone point me to Matt's patches? Is that this one: http://gcc.gnu.org/ml/gcc-patches/2012-09/msg00274.html or are there others? Thanks, Teresa > > I am out of office at the moment, so it's a little bit inconvenient to > investigate deeper the reasons for all the errors reported by Steven's > patch. Anyway it looks like it's really needed :) > I also noticed that some compilations failed with an ICE in calc_dfs_tree at > dominance.c:395. > > > Christophe. > -- Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
On 31 October 2012 20:06, Teresa Johnson <tejohnson@google.com> wrote: > On Wed, Oct 31, 2012 at 12:58 PM, Christophe Lyon > <christophe.lyon@st.com> wrote: >> On 30.10.2012 17:59, Teresa Johnson wrote: >>> >>> On Tue, Oct 30, 2012 at 9:26 AM, Steven Bosscher <stevenb.gcc@gmail.com> >>> wrote: >>>> >>>> Hello, >>>> >>>> Hot/cold partitioning is apparently a hot topic all of a sudden, which >>>> is a good thing of course, because it's in need of some TLC. >>>> >>>> The attached patch adds another check the RTL cfg checking >>>> (verify_flow_info) for the partitioning: A hot block can never be >>>> dominated by a cold block (because the dominated block must also be >>>> cold). This trips in PR55121. >>>> >>>> I haven't tested this with any profiling tests, but it's bound to >>>> break things. From my POV, whatever gets broken by this patch was >>>> already broken to begin with :-) If you're in CC, it's because I >>>> hope you can help test this patch. >>> >>> I will try testing your patch on top of mine with our fdo benchmarks. >>> For the others on the cc list, you may need to include my patch as >>> well for testing. Without it, -freorder-blocks-and-partition was DOA >>> for me. For my patch, see >>> http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02692.html >>> >>> Teresa >>> >> I have tried Steven's patch an indeed it reported numerous errors while >> compiling spec2k. >> >> I tried Teresa's patch too, but it changed nothing in my tests. The patches >> already posted by Matt are still necessary and Teresa's patch does not >> improve my tests. > > With checking enabled I am seeing additional failures that my fixes > are not addressing. Looking into those now. > Can someone point me to Matt's patches? Is that this one: > http://gcc.gnu.org/ml/gcc-patches/2012-09/msg00274.html > or are there others? That is one of two. The other one is: http://gcc.gnu.org/ml/gcc-patches/2012-09/msg00275.html I'd be careful with the second one - the original patch posted (in the link) fixes the issue I was seeing, but Stephen Bosscher suggested I made some changes, and I reposted a patch with some incorrect logic (which unfortunately also fixes the issue but more by luck than judgement). I haven't had the time to fully work through updating and testing a reworked patch yet. Thanks, Matt
Hello Teresa, It seems to me that it's better if you commit it along with your set of fixes. My patch doesn't fix any bugs, it just exposes them :-) Ciao! Steven On Fri, Nov 9, 2012 at 9:09 PM, Teresa Johnson <tejohnson@google.com> wrote: > > Hi Steven, > > I've spent this week trying to clean up all the issues exposed by this new verification patch. Some of the issues I described in the email thread on my related patch (http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00287.html) and earlier in this thread. It also exposed more issues of the type described in the last message regarding my patch (the link I included here), where transformations were being applied but the partitions not being correctly fixed up. Things look clean now across SPEC2006 int C benchmarks at peak, gcc regression tests and our internal benchmarks. I need to update from head, retest and clean things up though before sending the new patch. But do you want to go ahead and commit this patch? I guess it should be fine to commit asynchronously with mine since -freorder-blocks-and-partition is off by default and not working anyway. I assume it can still go in since it was proposed already and is related to some outstanding bugs? > > Thanks, > Teresa > > > On Wed, Oct 31, 2012 at 1:06 PM, Teresa Johnson <tejohnson@google.com> wrote: >> >> On Wed, Oct 31, 2012 at 12:58 PM, Christophe Lyon >> <christophe.lyon@st.com> wrote: >> > On 30.10.2012 17:59, Teresa Johnson wrote: >> >> >> >> On Tue, Oct 30, 2012 at 9:26 AM, Steven Bosscher <stevenb.gcc@gmail.com> >> >> wrote: >> >>> >> >>> Hello, >> >>> >> >>> Hot/cold partitioning is apparently a hot topic all of a sudden, which >> >>> is a good thing of course, because it's in need of some TLC. >> >>> >> >>> The attached patch adds another check the RTL cfg checking >> >>> (verify_flow_info) for the partitioning: A hot block can never be >> >>> dominated by a cold block (because the dominated block must also be >> >>> cold). This trips in PR55121. >> >>> >> >>> I haven't tested this with any profiling tests, but it's bound to >> >>> break things. From my POV, whatever gets broken by this patch was >> >>> already broken to begin with :-) If you're in CC, it's because I >> >>> hope you can help test this patch. >> >> >> >> I will try testing your patch on top of mine with our fdo benchmarks. >> >> For the others on the cc list, you may need to include my patch as >> >> well for testing. Without it, -freorder-blocks-and-partition was DOA >> >> for me. For my patch, see >> >> http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02692.html >> >> >> >> Teresa >> >> >> > I have tried Steven's patch an indeed it reported numerous errors while >> > compiling spec2k. >> > >> > I tried Teresa's patch too, but it changed nothing in my tests. The patches >> > already posted by Matt are still necessary and Teresa's patch does not >> > improve my tests. >> >> With checking enabled I am seeing additional failures that my fixes >> are not addressing. Looking into those now. >> Can someone point me to Matt's patches? Is that this one: >> http://gcc.gnu.org/ml/gcc-patches/2012-09/msg00274.html >> or are there others? >> >> Thanks, >> Teresa >> >> > >> > I am out of office at the moment, so it's a little bit inconvenient to >> > investigate deeper the reasons for all the errors reported by Steven's >> > patch. Anyway it looks like it's really needed :) >> > I also noticed that some compilations failed with an ICE in calc_dfs_tree at >> > dominance.c:395. >> > >> > >> > Christophe. >> > >> >> >> >> -- >> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 > > > > > -- > Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 >
Ok, thanks. Will do, with appropriate credit. =) Teresa On Fri, Nov 9, 2012 at 12:12 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > Hello Teresa, > > It seems to me that it's better if you commit it along with your set > of fixes. My patch doesn't fix any bugs, it just exposes them :-) > > Ciao! > Steven > > > > On Fri, Nov 9, 2012 at 9:09 PM, Teresa Johnson <tejohnson@google.com> wrote: >> >> Hi Steven, >> >> I've spent this week trying to clean up all the issues exposed by this new verification patch. Some of the issues I described in the email thread on my related patch (http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00287.html) and earlier in this thread. It also exposed more issues of the type described in the last message regarding my patch (the link I included here), where transformations were being applied but the partitions not being correctly fixed up. Things look clean now across SPEC2006 int C benchmarks at peak, gcc regression tests and our internal benchmarks. I need to update from head, retest and clean things up though before sending the new patch. But do you want to go ahead and commit this patch? I guess it should be fine to commit asynchronously with mine since -freorder-blocks-and-partition is off by default and not working anyway. I assume it can still go in since it was proposed already and is related to some outstanding bugs? >> >> Thanks, >> Teresa >> >> >> On Wed, Oct 31, 2012 at 1:06 PM, Teresa Johnson <tejohnson@google.com> wrote: >>> >>> On Wed, Oct 31, 2012 at 12:58 PM, Christophe Lyon >>> <christophe.lyon@st.com> wrote: >>> > On 30.10.2012 17:59, Teresa Johnson wrote: >>> >> >>> >> On Tue, Oct 30, 2012 at 9:26 AM, Steven Bosscher <stevenb.gcc@gmail.com> >>> >> wrote: >>> >>> >>> >>> Hello, >>> >>> >>> >>> Hot/cold partitioning is apparently a hot topic all of a sudden, which >>> >>> is a good thing of course, because it's in need of some TLC. >>> >>> >>> >>> The attached patch adds another check the RTL cfg checking >>> >>> (verify_flow_info) for the partitioning: A hot block can never be >>> >>> dominated by a cold block (because the dominated block must also be >>> >>> cold). This trips in PR55121. >>> >>> >>> >>> I haven't tested this with any profiling tests, but it's bound to >>> >>> break things. From my POV, whatever gets broken by this patch was >>> >>> already broken to begin with :-) If you're in CC, it's because I >>> >>> hope you can help test this patch. >>> >> >>> >> I will try testing your patch on top of mine with our fdo benchmarks. >>> >> For the others on the cc list, you may need to include my patch as >>> >> well for testing. Without it, -freorder-blocks-and-partition was DOA >>> >> for me. For my patch, see >>> >> http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02692.html >>> >> >>> >> Teresa >>> >> >>> > I have tried Steven's patch an indeed it reported numerous errors while >>> > compiling spec2k. >>> > >>> > I tried Teresa's patch too, but it changed nothing in my tests. The patches >>> > already posted by Matt are still necessary and Teresa's patch does not >>> > improve my tests. >>> >>> With checking enabled I am seeing additional failures that my fixes >>> are not addressing. Looking into those now. >>> Can someone point me to Matt's patches? Is that this one: >>> http://gcc.gnu.org/ml/gcc-patches/2012-09/msg00274.html >>> or are there others? >>> >>> Thanks, >>> Teresa >>> >>> > >>> > I am out of office at the moment, so it's a little bit inconvenient to >>> > investigate deeper the reasons for all the errors reported by Steven's >>> > patch. Anyway it looks like it's really needed :) >>> > I also noticed that some compilations failed with an ICE in calc_dfs_tree at >>> > dominance.c:395. >>> > >>> > >>> > Christophe. >>> > >>> >>> >>> >>> -- >>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 >> >> >> >> >> -- >> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 >>
Index: cfgrtl.c =================================================================== --- cfgrtl.c (revision 191819) +++ cfgrtl.c (working copy) @@ -2033,6 +2033,7 @@ rtl_verify_flow_info_1 (void) rtx x; int err = 0; basic_block bb; + bool have_partitions = false; /* Check the general integrity of the basic blocks. */ FOR_EACH_BB_REVERSE (bb) @@ -2145,6 +2146,8 @@ rtl_verify_flow_info_1 (void) n_eh++; else if (e->flags & EDGE_ABNORMAL) n_abnormal++; + + have_partitions |= is_crossing; } if (n_eh && !find_reg_note (BB_END (bb), REG_EH_REGION, NULL_RTX)) @@ -2263,6 +2268,40 @@ rtl_verify_flow_info_1 (void) } } + /* If there are partitions, do a sanity check on them: A basic block in + a cold partition cannot dominate a basic block in a hot partition. */ + VEC (basic_block, heap) *bbs_in_cold_partition = NULL; + if (have_partitions && !err) + FOR_EACH_BB (bb) + if ((BB_PARTITION (bb) == BB_COLD_PARTITION)) + VEC_safe_push (basic_block, heap, bbs_in_cold_partition, bb); + if (! VEC_empty (basic_block, bbs_in_cold_partition)) + { + bool dom_calculated_here = !dom_info_available_p (CDI_DOMINATORS); + basic_block son; + + if (dom_calculated_here) + calculate_dominance_info (CDI_DOMINATORS); + + while (! VEC_empty (basic_block, bbs_in_cold_partition)) + { + bb = VEC_pop (basic_block, bbs_in_cold_partition); + if ((BB_PARTITION (bb) != BB_COLD_PARTITION)) + { + error ("non-cold basic block %d dominated " + "by a block in the cold partition", bb->index); + err = 1; + } + for (son = first_dom_son (CDI_DOMINATORS, bb); + son; + son = next_dom_son (CDI_DOMINATORS, son)) + VEC_safe_push (basic_block, heap, bbs_in_cold_partition, son); + } + + if (dom_calculated_here) + free_dominance_info (CDI_DOMINATORS); + } + /* Clean up. */ return err; }