[RFC] Sanity checking for -freorder-blocks-and-partition failures

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.

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.

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
>>