rs6000: Fix lost ud chains in swap optimization

Message ID 08b57042-de7e-bf70-9468-cb0ee04c4963@linux.ibm.com
State New
Headers show
Series
  • rs6000: Fix lost ud chains in swap optimization
Related show

Commit Message

Bill Schmidt March 8, 2019, 12:33 a.m.
Hi,

We recently discovered a problem in swap optimization where the du- and ud-chains
were getting corrupted after a preliminary modification phase and prior to the
main body of the pass.  The fix for this is to rebuild the chains between phases.

Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions.
I've not included a test case because the problem tends to get lost in reduction,
and may shift over time anyway.  Is this okay for trunk, and eventual backport
to 8 and 7?

Thanks!

Bill


2019-03-07  Bill Schmidt  <wschmidt@linux.ibm.com>

	* config/rs6000/rs6000-p8swap.c (rs6000_analyze_swaps): Rebuild
	ud- and du-chains between phases.

Comments

Richard Biener March 8, 2019, 10:40 a.m. | #1
On Fri, Mar 8, 2019 at 1:34 AM Bill Schmidt <wschmidt@linux.ibm.com> wrote:
>

> Hi,

>

> We recently discovered a problem in swap optimization where the du- and ud-chains

> were getting corrupted after a preliminary modification phase and prior to the

> main body of the pass.  The fix for this is to rebuild the chains between phases.


It looks expensive - is it possible to keep them up-to-date instead?

> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions.

> I've not included a test case because the problem tends to get lost in reduction,

> and may shift over time anyway.  Is this okay for trunk, and eventual backport

> to 8 and 7?

>

> Thanks!

>

> Bill

>

>

> 2019-03-07  Bill Schmidt  <wschmidt@linux.ibm.com>

>

>         * config/rs6000/rs6000-p8swap.c (rs6000_analyze_swaps): Rebuild

>         ud- and du-chains between phases.

>

>

> Index: gcc/config/rs6000/rs6000-p8swap.c

> ===================================================================

> --- gcc/config/rs6000/rs6000-p8swap.c   (revision 269471)

> +++ gcc/config/rs6000/rs6000-p8swap.c   (working copy)

> @@ -2316,7 +2316,14 @@ rs6000_analyze_swaps (function *fun)

>

>    /* Pre-pass to recombine lvx and stvx patterns so we don't lose info.  */

>    recombine_lvx_stvx_patterns (fun);

> +

> +  /* Rebuild ud- and du-chains.  */

> +  df_remove_problem (df_chain);

>    df_process_deferred_rescans ();

> +  df_set_flags (DF_RD_PRUNE_DEAD_DEFS);

> +  df_chain_add_problem (DF_DU_CHAIN | DF_UD_CHAIN);

> +  df_analyze ();

> +  df_set_flags (DF_DEFER_INSN_RESCAN);

>

>    /* Allocate structure to represent webs of insns.  */

>    insn_entry = XCNEWVEC (swap_web_entry, get_max_uid ());

>
Bill Schmidt March 8, 2019, 12:35 p.m. | #2
On 3/8/19 4:40 AM, Richard Biener wrote:
> On Fri, Mar 8, 2019 at 1:34 AM Bill Schmidt <wschmidt@linux.ibm.com> wrote:

>> Hi,

>>

>> We recently discovered a problem in swap optimization where the du- and ud-chains

>> were getting corrupted after a preliminary modification phase and prior to the

>> main body of the pass.  The fix for this is to rebuild the chains between phases.

> It looks expensive - is it possible to keep them up-to-date instead?


That's what I've been doing up till now.  There appears to be a problem with
rescanning and the DF_DU_CHAIN and DF_UD_CHAIN dataflow problems.  Whether I
use df_process_deferred_rescans or df_insn_rescan_all, I see a problem where
now and then a use-def chain gets lost.

As I looked into it further, I found this comment on df_insn_rescan_all:

/* Rescan all of the insns in the function.  Note that the artificial           
   uses and defs are not touched.  This function will destroy def-use           
   or use-def chains.  */

So if that's accurate, it appears that the rescan machinery is not compatible
with du-/ud-chains, which is consistent with my experience.  This is the
second time we've encountered this issue.  It doesn't come up often, but
when it does, it's a real head-scratcher to debug.

If an expert in the df code thinks the above comment is erroneous, then there
is at least a subtle bug somewhere in the df code that would be good to fix.
But seeing the comment, I felt warned off completely from rescan.  This at
least allows us to fix the problem that the OpenCV community found.

Thanks,
Bill

>

>> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions.

>> I've not included a test case because the problem tends to get lost in reduction,

>> and may shift over time anyway.  Is this okay for trunk, and eventual backport

>> to 8 and 7?

>>

>> Thanks!

>>

>> Bill

>>

>>

>> 2019-03-07  Bill Schmidt  <wschmidt@linux.ibm.com>

>>

>>         * config/rs6000/rs6000-p8swap.c (rs6000_analyze_swaps): Rebuild

>>         ud- and du-chains between phases.

>>

>>

>> Index: gcc/config/rs6000/rs6000-p8swap.c

>> ===================================================================

>> --- gcc/config/rs6000/rs6000-p8swap.c   (revision 269471)

>> +++ gcc/config/rs6000/rs6000-p8swap.c   (working copy)

>> @@ -2316,7 +2316,14 @@ rs6000_analyze_swaps (function *fun)

>>

>>    /* Pre-pass to recombine lvx and stvx patterns so we don't lose info.  */

>>    recombine_lvx_stvx_patterns (fun);

>> +

>> +  /* Rebuild ud- and du-chains.  */

>> +  df_remove_problem (df_chain);

>>    df_process_deferred_rescans ();

>> +  df_set_flags (DF_RD_PRUNE_DEAD_DEFS);

>> +  df_chain_add_problem (DF_DU_CHAIN | DF_UD_CHAIN);

>> +  df_analyze ();

>> +  df_set_flags (DF_DEFER_INSN_RESCAN);

>>

>>    /* Allocate structure to represent webs of insns.  */

>>    insn_entry = XCNEWVEC (swap_web_entry, get_max_uid ());

>>
Richard Biener March 8, 2019, 1:53 p.m. | #3
On Fri, Mar 8, 2019 at 1:35 PM Bill Schmidt <wschmidt@linux.ibm.com> wrote:
>

> On 3/8/19 4:40 AM, Richard Biener wrote:

>

> On Fri, Mar 8, 2019 at 1:34 AM Bill Schmidt <wschmidt@linux.ibm.com> wrote:

>

> Hi,

>

> We recently discovered a problem in swap optimization where the du- and ud-chains

> were getting corrupted after a preliminary modification phase and prior to the

> main body of the pass.  The fix for this is to rebuild the chains between phases.

>

> It looks expensive - is it possible to keep them up-to-date instead?

>

> That's what I've been doing up till now.  There appears to be a problem with

> rescanning and the DF_DU_CHAIN and DF_UD_CHAIN dataflow problems.  Whether I

> use df_process_deferred_rescans or df_insn_rescan_all, I see a problem where

> now and then a use-def chain gets lost.

>

> As I looked into it further, I found this comment on df_insn_rescan_all:

>

> /* Rescan all of the insns in the function.  Note that the artificial

>    uses and defs are not touched.  This function will destroy def-use

>    or use-def chains.  */

>

> So if that's accurate, it appears that the rescan machinery is not compatible

> with du-/ud-chains, which is consistent with my experience.  This is the

> second time we've encountered this issue.  It doesn't come up often, but

> when it does, it's a real head-scratcher to debug.

>

> If an expert in the df code thinks the above comment is erroneous, then there

> is at least a subtle bug somewhere in the df code that would be good to fix.

> But seeing the comment, I felt warned off completely from rescan.  This at

> least allows us to fix the problem that the OpenCV community found.


OK, I see - I'm not very familiar with DF.

> Thanks,

> Bill

>

> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions.

> I've not included a test case because the problem tends to get lost in reduction,

> and may shift over time anyway.  Is this okay for trunk, and eventual backport

> to 8 and 7?

>

> Thanks!

>

> Bill

>

>

> 2019-03-07  Bill Schmidt  <wschmidt@linux.ibm.com>

>

>         * config/rs6000/rs6000-p8swap.c (rs6000_analyze_swaps): Rebuild

>         ud- and du-chains between phases.

>

>

> Index: gcc/config/rs6000/rs6000-p8swap.c

> ===================================================================

> --- gcc/config/rs6000/rs6000-p8swap.c   (revision 269471)

> +++ gcc/config/rs6000/rs6000-p8swap.c   (working copy)

> @@ -2316,7 +2316,14 @@ rs6000_analyze_swaps (function *fun)

>

>    /* Pre-pass to recombine lvx and stvx patterns so we don't lose info.  */

>    recombine_lvx_stvx_patterns (fun);

> +

> +  /* Rebuild ud- and du-chains.  */

> +  df_remove_problem (df_chain);

>    df_process_deferred_rescans ();

> +  df_set_flags (DF_RD_PRUNE_DEAD_DEFS);

> +  df_chain_add_problem (DF_DU_CHAIN | DF_UD_CHAIN);

> +  df_analyze ();

> +  df_set_flags (DF_DEFER_INSN_RESCAN);

>

>    /* Allocate structure to represent webs of insns.  */

>    insn_entry = XCNEWVEC (swap_web_entry, get_max_uid ());

>

>
Segher Boessenkool March 8, 2019, 4:48 p.m. | #4
Hi Bill,

On Thu, Mar 07, 2019 at 06:33:48PM -0600, Bill Schmidt wrote:
> We recently discovered a problem in swap optimization where the du- and ud-chains

> were getting corrupted after a preliminary modification phase and prior to the

> main body of the pass.  The fix for this is to rebuild the chains between phases.

> 

> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions.

> I've not included a test case because the problem tends to get lost in reduction,

> and may shift over time anyway.  Is this okay for trunk, and eventual backport

> to 8 and 7?


Like Richard I think this is expensive.  But I don't think you can do better
currently, so, okay for trunk and backports.  Thanks!


Segher

Patch

Index: gcc/config/rs6000/rs6000-p8swap.c
===================================================================
--- gcc/config/rs6000/rs6000-p8swap.c	(revision 269471)
+++ gcc/config/rs6000/rs6000-p8swap.c	(working copy)
@@ -2316,7 +2316,14 @@  rs6000_analyze_swaps (function *fun)
 
   /* Pre-pass to recombine lvx and stvx patterns so we don't lose info.  */
   recombine_lvx_stvx_patterns (fun);
+
+  /* Rebuild ud- and du-chains.  */
+  df_remove_problem (df_chain);
   df_process_deferred_rescans ();
+  df_set_flags (DF_RD_PRUNE_DEAD_DEFS);
+  df_chain_add_problem (DF_DU_CHAIN | DF_UD_CHAIN);
+  df_analyze ();
+  df_set_flags (DF_DEFER_INSN_RESCAN);
 
   /* Allocate structure to represent webs of insns.  */
   insn_entry = XCNEWVEC (swap_web_entry, get_max_uid ());