Patchwork cfglayout fixes

login
register
mail settings
Submitter Steven Bosscher
Date Dec. 6, 2012, 12:48 p.m.
Message ID <CABu31nMAN=-q1_ThsJK2Ugd=Zh-vFi4C8OYcEeJhrsE7tinz6w@mail.gmail.com>
Download mbox | patch
Permalink /patch/204217/
State New
Headers show

Comments

Steven Bosscher - Dec. 6, 2012, 12:48 p.m.
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.
Steven Bosscher - Dec. 11, 2012, 9:22 p.m.
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;
Marek Polacek - Dec. 11, 2012, 9:51 p.m.
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
Jakub Jelinek - Dec. 12, 2012, 10:17 a.m.
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
Steven Bosscher - Dec. 12, 2012, 10:29 a.m.
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
Jakub Jelinek - Dec. 12, 2012, 10:48 a.m.
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
Steven Bosscher - Dec. 12, 2012, 12:17 p.m.
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
Steven Bosscher - Dec. 12, 2012, 12:41 p.m.
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
Jakub Jelinek - Dec. 12, 2012, 4:18 p.m.
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

Patch

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;