[debug] Reuse debug exprs generated in remap_ssa_name

Message ID 20180708092725.c7ahp6ggfh6d3mqz@delia
State New
Headers show
Series
  • [debug] Reuse debug exprs generated in remap_ssa_name
Related show

Commit Message

Tom de Vries July 8, 2018, 9:27 a.m.
On Sun, Jul 08, 2018 at 11:22:41AM +0200, Tom de Vries wrote:
> On Fri, Jul 06, 2018 at 04:38:50PM +0200, Richard Biener wrote:

> > On Fri, Jul 6, 2018 at 12:47 PM Tom de Vries <tdevries@suse.de> wrote:

> > > On 07/05/2018 01:39 PM, Richard Biener wrote:

> 

> <SNIP>

> 

> > I now also spotted the code in remap_ssa_name that is supposed to handle

> > this it seems and for the testcase we only give up because the PARM_DECL is

> > remapped to a VAR_DECL.  So I suppose it is to be handled via the

> > debug-args stuff

> > which probably lacks in the area of versioning.

> > 

> > Your patch feels like it adds stuff ontop of existing mechanisms that

> > should "just work"

> > with the correct setup at the correct places...

> > 

> 

> Hmm, I realized that I may be complicating things, by trying to do an

> optimal fix in a single patch, so I decided to write two patches, one

> with a fix, and then one improving the fix to be more optimal.

> 

> Also, I suspect that the "just work" approach is this:

> ...

>        # DEBUG D#8 s=> iD.1900

>        # DEBUG iD.1949 => D#8

>        # DEBUG D#6 s=> iD.1949

> ...

> whereas previously I tried to map 'D#6' on iD.1900 directly.

> 


Second patch OK for trunk?

Thanks,
- Tom

[debug] Reuse debug exprs generated in remap_ssa_name

When compiling gcc.dg/vla-1.c with -O3 -g, vla a and b in f1 are optimized
away, and f1 is cloned to a version f1.constprop with no parameters, eliminating
parameter i.  Debug info is generated to describe the sizes of a and b, but
that process generates debug expressions that are not reused.

Fix the duplication by saving and reusing the generated debug expressions in
remap_ssa_name.  Concretely: reuse D#7 here instead of generating D#8:
...
 __attribute__((noinline))
 f1.constprop ()
 {
   intD.6 iD.1935;

   <bb 3>
   # DEBUG D#10 s=> iD.1897
   # DEBUG iD.1935 => D#10

   <bb 2>
-  # DEBUG D#8 s=> iD.1935
   # DEBUG D#7 s=> iD.1935
   saved_stack.2_1 = __builtin_stack_save ();
   # DEBUG BEGIN_STMT
   # DEBUG D#6 => D#7 + 1
   # DEBUG D#5 => (long intD.8) D#6
   # DEBUG D#4 => D#5 + -1
   # DEBUG D.1937 => (sizetype) D#4
   # DEBUG a.0D.1942 => NULL
   # DEBUG BEGIN_STMT
-  # DEBUG D#3 => D#8 + 2
+  # DEBUG D#3 => D#7 + 2
   # DEBUG D#2 => (long intD.8) D#3
   # DEBUG D#1 => D#2 + -1
   # DEBUG D.1944 => (sizetype) D#1
   # DEBUG b.1D.1949 => NULL
...

Bootstrapped and reg-tested on x86_64.

2018-07-07  Tom de Vries  <tdevries@suse.de>

	* tree-inline.c (remap_ssa_name): Save and reuse debug exprs generated
	in remap_ssa_name.

	* gcc.dg/vla-1.c: Update.

---
 gcc/testsuite/gcc.dg/vla-1.c | 5 +++--
 gcc/tree-inline.c            | 4 ++++
 2 files changed, 7 insertions(+), 2 deletions(-)

Comments

Richard Biener July 9, 2018, 12:43 p.m. | #1
On Sun, Jul 8, 2018 at 11:27 AM Tom de Vries <tdevries@suse.de> wrote:
>

> On Sun, Jul 08, 2018 at 11:22:41AM +0200, Tom de Vries wrote:

> > On Fri, Jul 06, 2018 at 04:38:50PM +0200, Richard Biener wrote:

> > > On Fri, Jul 6, 2018 at 12:47 PM Tom de Vries <tdevries@suse.de> wrote:

> > > > On 07/05/2018 01:39 PM, Richard Biener wrote:

> >

> > <SNIP>

> >

> > > I now also spotted the code in remap_ssa_name that is supposed to handle

> > > this it seems and for the testcase we only give up because the PARM_DECL is

> > > remapped to a VAR_DECL.  So I suppose it is to be handled via the

> > > debug-args stuff

> > > which probably lacks in the area of versioning.

> > >

> > > Your patch feels like it adds stuff ontop of existing mechanisms that

> > > should "just work"

> > > with the correct setup at the correct places...

> > >

> >

> > Hmm, I realized that I may be complicating things, by trying to do an

> > optimal fix in a single patch, so I decided to write two patches, one

> > with a fix, and then one improving the fix to be more optimal.

> >

> > Also, I suspect that the "just work" approach is this:

> > ...

> >        # DEBUG D#8 s=> iD.1900

> >        # DEBUG iD.1949 => D#8

> >        # DEBUG D#6 s=> iD.1949

> > ...

> > whereas previously I tried to map 'D#6' on iD.1900 directly.

> >

>

> Second patch OK for trunk?


OK, though I wonder how it doesn't fail with that testcase with
the mismatching type where the removed param-decl is mapped
to a local var-decl.

Richard.

> Thanks,

> - Tom

>

> [debug] Reuse debug exprs generated in remap_ssa_name

>

> When compiling gcc.dg/vla-1.c with -O3 -g, vla a and b in f1 are optimized

> away, and f1 is cloned to a version f1.constprop with no parameters, eliminating

> parameter i.  Debug info is generated to describe the sizes of a and b, but

> that process generates debug expressions that are not reused.

>

> Fix the duplication by saving and reusing the generated debug expressions in

> remap_ssa_name.  Concretely: reuse D#7 here instead of generating D#8:

> ...

>  __attribute__((noinline))

>  f1.constprop ()

>  {

>    intD.6 iD.1935;

>

>    <bb 3>

>    # DEBUG D#10 s=> iD.1897

>    # DEBUG iD.1935 => D#10

>

>    <bb 2>

> -  # DEBUG D#8 s=> iD.1935

>    # DEBUG D#7 s=> iD.1935

>    saved_stack.2_1 = __builtin_stack_save ();

>    # DEBUG BEGIN_STMT

>    # DEBUG D#6 => D#7 + 1

>    # DEBUG D#5 => (long intD.8) D#6

>    # DEBUG D#4 => D#5 + -1

>    # DEBUG D.1937 => (sizetype) D#4

>    # DEBUG a.0D.1942 => NULL

>    # DEBUG BEGIN_STMT

> -  # DEBUG D#3 => D#8 + 2

> +  # DEBUG D#3 => D#7 + 2

>    # DEBUG D#2 => (long intD.8) D#3

>    # DEBUG D#1 => D#2 + -1

>    # DEBUG D.1944 => (sizetype) D#1

>    # DEBUG b.1D.1949 => NULL

> ...

>

> Bootstrapped and reg-tested on x86_64.

>

> 2018-07-07  Tom de Vries  <tdevries@suse.de>

>

>         * tree-inline.c (remap_ssa_name): Save and reuse debug exprs generated

>         in remap_ssa_name.

>

>         * gcc.dg/vla-1.c: Update.

>

> ---

>  gcc/testsuite/gcc.dg/vla-1.c | 5 +++--

>  gcc/tree-inline.c            | 4 ++++

>  2 files changed, 7 insertions(+), 2 deletions(-)

>

> diff --git a/gcc/testsuite/gcc.dg/vla-1.c b/gcc/testsuite/gcc.dg/vla-1.c

> index 0c19feffd2b..94db23d1336 100644

> --- a/gcc/testsuite/gcc.dg/vla-1.c

> +++ b/gcc/testsuite/gcc.dg/vla-1.c

> @@ -20,6 +20,7 @@ main ()

>    return 0;

>  }

>

> -/* One debug source bind is generated for the parameter, and two to describe the

> +/* One debug source bind is generated for the parameter, and one to describe the

>     sizes of a and b.  */

> -/* { dg-final { scan-tree-dump-times " s=> i" 3 "optimized" } } */

> +/* { dg-final { scan-tree-dump-times " s=> i" 2 "optimized" } } */

> +

> diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c

> index 6fbd8c3ca61..164c7fff710 100644

> --- a/gcc/tree-inline.c

> +++ b/gcc/tree-inline.c

> @@ -215,12 +215,16 @@ remap_ssa_name (tree name, copy_body_data *id)

>               processing_debug_stmt = -1;

>               return name;

>             }

> +         n = id->decl_map->get (val);

> +         if (n && TREE_CODE (*n) == DEBUG_EXPR_DECL)

> +           return *n;

>           def_temp = gimple_build_debug_source_bind (vexpr, val, NULL);

>           DECL_ARTIFICIAL (vexpr) = 1;

>           TREE_TYPE (vexpr) = TREE_TYPE (name);

>           SET_DECL_MODE (vexpr, DECL_MODE (SSA_NAME_VAR (name)));

>           gsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)));

>           gsi_insert_before (&gsi, def_temp, GSI_SAME_STMT);

> +         insert_decl_map (id, val, vexpr);

>           return vexpr;

>         }

>

>
Tom de Vries July 13, 2018, 10:09 a.m. | #2
On 07/09/2018 02:43 PM, Richard Biener wrote:
> On Sun, Jul 8, 2018 at 11:27 AM Tom de Vries <tdevries@suse.de> wrote:

>>

>> On Sun, Jul 08, 2018 at 11:22:41AM +0200, Tom de Vries wrote:

>>> On Fri, Jul 06, 2018 at 04:38:50PM +0200, Richard Biener wrote:

>>>> On Fri, Jul 6, 2018 at 12:47 PM Tom de Vries <tdevries@suse.de> wrote:

>>>>> On 07/05/2018 01:39 PM, Richard Biener wrote:

>>>

>>> <SNIP>

>>>

>>>> I now also spotted the code in remap_ssa_name that is supposed to handle

>>>> this it seems and for the testcase we only give up because the PARM_DECL is

>>>> remapped to a VAR_DECL.  So I suppose it is to be handled via the

>>>> debug-args stuff

>>>> which probably lacks in the area of versioning.

>>>>

>>>> Your patch feels like it adds stuff ontop of existing mechanisms that

>>>> should "just work"

>>>> with the correct setup at the correct places...

>>>>

>>>

>>> Hmm, I realized that I may be complicating things, by trying to do an

>>> optimal fix in a single patch, so I decided to write two patches, one

>>> with a fix, and then one improving the fix to be more optimal.

>>>

>>> Also, I suspect that the "just work" approach is this:

>>> ...

>>>        # DEBUG D#8 s=> iD.1900

>>>        # DEBUG iD.1949 => D#8

>>>        # DEBUG D#6 s=> iD.1949

>>> ...

>>> whereas previously I tried to map 'D#6' on iD.1900 directly.

>>>

>>

>> Second patch OK for trunk?

> 

> OK, though I wonder how it doesn't fail with that testcase with

> the mismatching type where the removed param-decl is mapped

> to a local var-decl.


Previously I mapped the default def ssa-name onto the debug expression,
which meant I had to add special handling at the start of
remap_ssa_name, where already mapped ssa-names are handled, for the
mismatched argument/parameter type case.

Now I map the local var-decl onto the debug expression, and the code
that is added to look it up is already guarded with processing_debug_stmt.

Thanks,
- Tom

>> diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c

>> index 6fbd8c3ca61..164c7fff710 100644

>> --- a/gcc/tree-inline.c

>> +++ b/gcc/tree-inline.c

>> @@ -215,12 +215,16 @@ remap_ssa_name (tree name, copy_body_data *id)

>>               processing_debug_stmt = -1;

>>               return name;

>>             }

>> +         n = id->decl_map->get (val);

>> +         if (n && TREE_CODE (*n) == DEBUG_EXPR_DECL)

>> +           return *n;

>>           def_temp = gimple_build_debug_source_bind (vexpr, val, NULL);

>>           DECL_ARTIFICIAL (vexpr) = 1;

>>           TREE_TYPE (vexpr) = TREE_TYPE (name);

>>           SET_DECL_MODE (vexpr, DECL_MODE (SSA_NAME_VAR (name)));

>>           gsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)));

>>           gsi_insert_before (&gsi, def_temp, GSI_SAME_STMT);

>> +         insert_decl_map (id, val, vexpr);

>>           return vexpr;

>>         }

>>

>>
Richard Biener July 13, 2018, 11:10 a.m. | #3
On Fri, Jul 13, 2018 at 12:09 PM Tom de Vries <tdevries@suse.de> wrote:
>

> On 07/09/2018 02:43 PM, Richard Biener wrote:

> > On Sun, Jul 8, 2018 at 11:27 AM Tom de Vries <tdevries@suse.de> wrote:

> >>

> >> On Sun, Jul 08, 2018 at 11:22:41AM +0200, Tom de Vries wrote:

> >>> On Fri, Jul 06, 2018 at 04:38:50PM +0200, Richard Biener wrote:

> >>>> On Fri, Jul 6, 2018 at 12:47 PM Tom de Vries <tdevries@suse.de> wrote:

> >>>>> On 07/05/2018 01:39 PM, Richard Biener wrote:

> >>>

> >>> <SNIP>

> >>>

> >>>> I now also spotted the code in remap_ssa_name that is supposed to handle

> >>>> this it seems and for the testcase we only give up because the PARM_DECL is

> >>>> remapped to a VAR_DECL.  So I suppose it is to be handled via the

> >>>> debug-args stuff

> >>>> which probably lacks in the area of versioning.

> >>>>

> >>>> Your patch feels like it adds stuff ontop of existing mechanisms that

> >>>> should "just work"

> >>>> with the correct setup at the correct places...

> >>>>

> >>>

> >>> Hmm, I realized that I may be complicating things, by trying to do an

> >>> optimal fix in a single patch, so I decided to write two patches, one

> >>> with a fix, and then one improving the fix to be more optimal.

> >>>

> >>> Also, I suspect that the "just work" approach is this:

> >>> ...

> >>>        # DEBUG D#8 s=> iD.1900

> >>>        # DEBUG iD.1949 => D#8

> >>>        # DEBUG D#6 s=> iD.1949

> >>> ...

> >>> whereas previously I tried to map 'D#6' on iD.1900 directly.

> >>>

> >>

> >> Second patch OK for trunk?

> >

> > OK, though I wonder how it doesn't fail with that testcase with

> > the mismatching type where the removed param-decl is mapped

> > to a local var-decl.

>

> Previously I mapped the default def ssa-name onto the debug expression,

> which meant I had to add special handling at the start of

> remap_ssa_name, where already mapped ssa-names are handled, for the

> mismatched argument/parameter type case.

>

> Now I map the local var-decl onto the debug expression, and the code

> that is added to look it up is already guarded with processing_debug_stmt.


Ah, thanks for the explanation!

Richard.

> Thanks,

> - Tom

>

> >> diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c

> >> index 6fbd8c3ca61..164c7fff710 100644

> >> --- a/gcc/tree-inline.c

> >> +++ b/gcc/tree-inline.c

> >> @@ -215,12 +215,16 @@ remap_ssa_name (tree name, copy_body_data *id)

> >>               processing_debug_stmt = -1;

> >>               return name;

> >>             }

> >> +         n = id->decl_map->get (val);

> >> +         if (n && TREE_CODE (*n) == DEBUG_EXPR_DECL)

> >> +           return *n;

> >>           def_temp = gimple_build_debug_source_bind (vexpr, val, NULL);

> >>           DECL_ARTIFICIAL (vexpr) = 1;

> >>           TREE_TYPE (vexpr) = TREE_TYPE (name);

> >>           SET_DECL_MODE (vexpr, DECL_MODE (SSA_NAME_VAR (name)));

> >>           gsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)));

> >>           gsi_insert_before (&gsi, def_temp, GSI_SAME_STMT);

> >> +         insert_decl_map (id, val, vexpr);

> >>           return vexpr;

> >>         }

> >>

> >>

Patch

diff --git a/gcc/testsuite/gcc.dg/vla-1.c b/gcc/testsuite/gcc.dg/vla-1.c
index 0c19feffd2b..94db23d1336 100644
--- a/gcc/testsuite/gcc.dg/vla-1.c
+++ b/gcc/testsuite/gcc.dg/vla-1.c
@@ -20,6 +20,7 @@  main ()
   return 0;
 }
 
-/* One debug source bind is generated for the parameter, and two to describe the
+/* One debug source bind is generated for the parameter, and one to describe the
    sizes of a and b.  */
-/* { dg-final { scan-tree-dump-times " s=> i" 3 "optimized" } } */
+/* { dg-final { scan-tree-dump-times " s=> i" 2 "optimized" } } */
+
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 6fbd8c3ca61..164c7fff710 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -215,12 +215,16 @@  remap_ssa_name (tree name, copy_body_data *id)
 	      processing_debug_stmt = -1;
 	      return name;
 	    }
+	  n = id->decl_map->get (val);
+	  if (n && TREE_CODE (*n) == DEBUG_EXPR_DECL)
+	    return *n;
 	  def_temp = gimple_build_debug_source_bind (vexpr, val, NULL);
 	  DECL_ARTIFICIAL (vexpr) = 1;
 	  TREE_TYPE (vexpr) = TREE_TYPE (name);
 	  SET_DECL_MODE (vexpr, DECL_MODE (SSA_NAME_VAR (name)));
 	  gsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
 	  gsi_insert_before (&gsi, def_temp, GSI_SAME_STMT);
+	  insert_decl_map (id, val, vexpr);
 	  return vexpr;
 	}