Patchwork Fix PR 53743 and other -freorder-blocks-and-partition failures (issue6823047)

login
register
mail settings
Submitter Steven Bosscher
Date Oct. 30, 2012, 9:33 p.m.
Message ID <CABu31nPV=+b=1iZG4nXsoE_u4PtOcWMG3V9n3+9zabkvfxBkaw@mail.gmail.com>
Download mbox | patch
Permalink /patch/195630/
State New
Headers show

Comments

Steven Bosscher - Oct. 30, 2012, 9:33 p.m.
On Tue, Oct 30, 2012 at 10:28 PM, Steven Bosscher wrote:
> Hello Teresa,
>
> Could you try this patch for me also? It moves bbpart outside the part
> of the passes pipeline that works in cfglayout mode.

where's the "unsend" button if you need it...

So, to complete the mail...

Could you try this patch for me also? It moves bbpart outside the part
of the passes pipeline that works in cfglayout mode. It looks like
when someone (/me looks the other way) changed the compiler to work
like that, he/she forgot about updating this pass...

Instead, the pass should run just before register allocation, as late
as possible so that any funny CFG modifications have taken place. A
possible down-side is that profile info may have degenerated a bit
further, but I don't think that's a serious concern because the
partitioning is actually quite stupid: Just stuff all blocks with
count==0 into the cold section. Updating 0-counts is easy enough that
I think GCC should get that right everywhere.

It'd be nice to figure out if there are less stupid^Wsimplistic
heuristics to decide what should go into the cold partition, for GCC
4.9...

Ciao!
Steven


        * passes.c (init_optimization_passes): Move
pass_partition_blocks just before RA.
Teresa Johnson - Oct. 30, 2012, 9:43 p.m.
On Tue, Oct 30, 2012 at 2:33 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Tue, Oct 30, 2012 at 10:28 PM, Steven Bosscher wrote:
>> Hello Teresa,
>>
>> Could you try this patch for me also? It moves bbpart outside the part
>> of the passes pipeline that works in cfglayout mode.
>
> where's the "unsend" button if you need it...
>
> So, to complete the mail...
>
> Could you try this patch for me also? It moves bbpart outside the part
> of the passes pipeline that works in cfglayout mode. It looks like
> when someone (/me looks the other way) changed the compiler to work
> like that, he/she forgot about updating this pass...

Sure, I will give this a try after your verification patch tests
complete. Does this mean that the patch you posted above to
force_nonfallthru_and_redirect is no longer needed either? I'll see if
I can avoid the need for some of my fixes, although I believe at least
the function.c one will still be needed. I'll check.

Regarding your earlier question about why we needed to add the
barrier, I need to dig up the details again but essentially I found
that the barriers were being added by bbpart, but bbro was reordering
things and the block that ended up at the border between the hot and
cold section didn't necessarily have a barrier on it because it was
not previously at the region boundary.


>
> Instead, the pass should run just before register allocation, as late
> as possible so that any funny CFG modifications have taken place. A
> possible down-side is that profile info may have degenerated a bit
> further, but I don't think that's a serious concern because the
> partitioning is actually quite stupid: Just stuff all blocks with
> count==0 into the cold section. Updating 0-counts is easy enough that
> I think GCC should get that right everywhere.
>
> It'd be nice to figure out if there are less stupid^Wsimplistic
> heuristics to decide what should go into the cold partition, for GCC
> 4.9...

Yep, that is one of my goals in looking at this - I am hoping to use
the new fdo summary info I added to tune this into a more robust
decision based on the counter values in the working set summary.

Thanks for the help!
Teresa

>
> Ciao!
> Steven
>
>
>         * passes.c (init_optimization_passes): Move
> pass_partition_blocks just before RA.
>
> Index: passes.c
> ===================================================================
> --- passes.c    (revision 192995)
> +++ passes.c    (working copy)
> @@ -1595,7 +1595,6 @@ init_optimization_passes (void)
>        NEXT_PASS (pass_ud_rtl_dce);
>        NEXT_PASS (pass_combine);
>        NEXT_PASS (pass_if_after_combine);
> -      NEXT_PASS (pass_partition_blocks);
>        NEXT_PASS (pass_regmove);
>        NEXT_PASS (pass_outof_cfg_layout_mode);
>        NEXT_PASS (pass_split_all_insns);
> @@ -1606,6 +1605,7 @@ init_optimization_passes (void)
>        NEXT_PASS (pass_match_asm_constraints);
>        NEXT_PASS (pass_sms);
>        NEXT_PASS (pass_sched);
> +      NEXT_PASS (pass_partition_blocks);
>        NEXT_PASS (pass_ira);
>        NEXT_PASS (pass_reload);
>        NEXT_PASS (pass_postreload);



--
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
Teresa Johnson - Oct. 31, 2012, 5:13 a.m.
On Tue, Oct 30, 2012 at 2:33 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Tue, Oct 30, 2012 at 10:28 PM, Steven Bosscher wrote:
>> Hello Teresa,
>>
>> Could you try this patch for me also? It moves bbpart outside the part
>> of the passes pipeline that works in cfglayout mode.
>
> where's the "unsend" button if you need it...
>
> So, to complete the mail...
>
> Could you try this patch for me also? It moves bbpart outside the part
> of the passes pipeline that works in cfglayout mode. It looks like
> when someone (/me looks the other way) changed the compiler to work
> like that, he/she forgot about updating this pass...
>
> Instead, the pass should run just before register allocation, as late
> as possible so that any funny CFG modifications have taken place. A
> possible down-side is that profile info may have degenerated a bit
> further, but I don't think that's a serious concern because the
> partitioning is actually quite stupid: Just stuff all blocks with
> count==0 into the cold section. Updating 0-counts is easy enough that
> I think GCC should get that right everywhere.
>
> It'd be nice to figure out if there are less stupid^Wsimplistic
> heuristics to decide what should go into the cold partition, for GCC
> 4.9...
>
> Ciao!
> Steven
>
>
>         * passes.c (init_optimization_passes): Move
> pass_partition_blocks just before RA.
>
> Index: passes.c
> ===================================================================
> --- passes.c    (revision 192995)
> +++ passes.c    (working copy)
> @@ -1595,7 +1595,6 @@ init_optimization_passes (void)
>        NEXT_PASS (pass_ud_rtl_dce);
>        NEXT_PASS (pass_combine);
>        NEXT_PASS (pass_if_after_combine);
> -      NEXT_PASS (pass_partition_blocks);
>        NEXT_PASS (pass_regmove);
>        NEXT_PASS (pass_outof_cfg_layout_mode);
>        NEXT_PASS (pass_split_all_insns);
> @@ -1606,6 +1605,7 @@ init_optimization_passes (void)
>        NEXT_PASS (pass_match_asm_constraints);
>        NEXT_PASS (pass_sms);
>        NEXT_PASS (pass_sched);
> +      NEXT_PASS (pass_partition_blocks);
>        NEXT_PASS (pass_ira);
>        NEXT_PASS (pass_reload);
>        NEXT_PASS (pass_postreload);


This doesn't quite work for me. I got an error because bbpart has
PROP_cfglayout listed in its required properties. I tried removing
that, but then verify_flow_info gives an error about a missing barrier
after a bb with no fall through edges.

Teresa

Patch

Index: passes.c
===================================================================
--- passes.c	(revision 192995)
+++ passes.c	(working copy)
@@ -1595,7 +1595,6 @@  init_optimization_passes (void)
       NEXT_PASS (pass_ud_rtl_dce);
       NEXT_PASS (pass_combine);
       NEXT_PASS (pass_if_after_combine);
-      NEXT_PASS (pass_partition_blocks);
       NEXT_PASS (pass_regmove);
       NEXT_PASS (pass_outof_cfg_layout_mode);
       NEXT_PASS (pass_split_all_insns);
@@ -1606,6 +1605,7 @@  init_optimization_passes (void)
       NEXT_PASS (pass_match_asm_constraints);
       NEXT_PASS (pass_sms);
       NEXT_PASS (pass_sched);
+      NEXT_PASS (pass_partition_blocks);
       NEXT_PASS (pass_ira);
       NEXT_PASS (pass_reload);
       NEXT_PASS (pass_postreload);