On Tue, Oct 30, 2012 at 2:31 PM, Teresa Johnson <tejohn...@google.com> wrote: > On Tue, Oct 30, 2012 at 12:33 PM, Xinliang David Li <davi...@google.com> > wrote: >> On Tue, Oct 30, 2012 at 12:25 PM, Steven Bosscher <stevenb....@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 | tejohn...@google.com | 408-460-2413 -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413
