[PATCH] Avoid creating useless debug temporaries

[PATCH] Avoid creating useless debug temporaries

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 536 bytes --]

Hi,

insert_debug_temp_for_var_def has some strange code whereby it creates debug 
temporaries for SINGLE_RHS (RHS for gimple_assign_single_p) but not for other 
RHS in the same situation.  Removing it saves 25% of compilation time at -g -O 
for a pathological testcase I have.

Bootstrapped/regtested on x86-64/Linux, OK for the mainline?


2023-04-25  Eric Botcazou  <ebotcazou@adacore.com>

	* tree-ssa.cc (insert_debug_temp_for_var_def): Do not create
	superfluous debug temporaries for single GIMPLE assignments.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 1726 bytes --]

diff --git a/gcc/tree-ssa.cc b/gcc/tree-ssa.cc
index a5cad2d344e..4ca1f5f3104 100644
--- a/gcc/tree-ssa.cc
+++ b/gcc/tree-ssa.cc
@@ -412,8 +412,7 @@ insert_debug_temp_for_var_def (gimple_stmt_iterator *gsi, tree var)
     {
       /* If there's a single use of VAR, and VAR is the entire debug
 	 expression (usecount would have been incremented again
-	 otherwise), and the definition involves only constants and
-	 SSA names, then we can propagate VALUE into this single use,
+	 otherwise), then we can propagate VALUE into this single use,
 	 avoiding the temp.
 
 	 We can also avoid using a temp if VALUE can be shared and
@@ -424,11 +423,9 @@ insert_debug_temp_for_var_def (gimple_stmt_iterator *gsi, tree var)
 	 are deferred to a debug temp, although we could avoid temps
 	 at the expense of duplication of expressions.  */
 
-      if (CONSTANT_CLASS_P (value)
+      if (usecount == 1
 	  || gimple_code (def_stmt) == GIMPLE_PHI
-	  || (usecount == 1
-	      && (!gimple_assign_single_p (def_stmt)
-		  || is_gimple_min_invariant (value)))
+	  || CONSTANT_CLASS_P (value)
 	  || is_gimple_reg (value))
 	;
       else
@@ -466,11 +463,6 @@ insert_debug_temp_for_var_def (gimple_stmt_iterator *gsi, tree var)
       if (value)
 	{
 	  FOR_EACH_IMM_USE_ON_STMT (use_p, imm_iter)
-	    /* unshare_expr is not needed here.  vexpr is either a
-	       SINGLE_RHS, that can be safely shared, some other RHS
-	       that was unshared when we found it had a single debug
-	       use, or a DEBUG_EXPR_DECL, that can be safely
-	       shared.  */
 	    SET_USE (use_p, unshare_expr (value));
 	  /* If we didn't replace uses with a debug decl fold the
 	     resulting expression.  Otherwise we end up with invalid IL.  */

Re: [PATCH] Avoid creating useless debug temporaries

[Richard Biener]

On Tue, Apr 25, 2023 at 11:34 AM Eric Botcazou via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
>
> insert_debug_temp_for_var_def has some strange code whereby it creates debug
> temporaries for SINGLE_RHS (RHS for gimple_assign_single_p) but not for other
> RHS in the same situation.

It indeed looks odd (likewise the GIMPLE_PHI handling should be
covered by is_gimple_reg ()).

> Removing it saves 25% of compilation time at -g -O
> for a pathological testcase I have.
>
> Bootstrapped/regtested on x86-64/Linux, OK for the mainline?

OK.

probably also helps PR109612 and the other similar PR referenced therein.

Thanks,
Richard.

>
> 2023-04-25  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * tree-ssa.cc (insert_debug_temp_for_var_def): Do not create
>         superfluous debug temporaries for single GIMPLE assignments.
>
> --
> Eric Botcazou

Re: [PATCH] Avoid creating useless debug temporaries

[Jakub Jelinek]

On Tue, Apr 25, 2023 at 05:10:50PM +0200, Richard Biener via Gcc-patches wrote:
> On Tue, Apr 25, 2023 at 11:34 AM Eric Botcazou via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > Hi,
> >
> > insert_debug_temp_for_var_def has some strange code whereby it creates debug
> > temporaries for SINGLE_RHS (RHS for gimple_assign_single_p) but not for other
> > RHS in the same situation.
> 
> It indeed looks odd (likewise the GIMPLE_PHI handling should be
> covered by is_gimple_reg ()).
> 
> > Removing it saves 25% of compilation time at -g -O
> > for a pathological testcase I have.
> >
> > Bootstrapped/regtested on x86-64/Linux, OK for the mainline?
> 
> OK.
> 
> probably also helps PR109612 and the other similar PR referenced therein.

Haven't looked into detail, but just saving compilation time shouldn't be
the only factor when deciding about debug info stuff, another and perhaps
even more important would be whether it affects the emitted debug info.

One of ways to quantify that was running https://github.com/pmachata/dwlocstat
before/after the change on something large, e.g. bootstrapped cc1plus
(of course with the patch reverted and stage3 rebuilt so that it is the same
code just with possibly different debug info).

	Jakub

Re: [PATCH] Avoid creating useless debug temporaries

[Eric Botcazou]

> Haven't looked into detail, but just saving compilation time shouldn't be
> the only factor when deciding about debug info stuff, another and perhaps
> even more important would be whether it affects the emitted debug info.

At least it doesn't change the guality results.

-- 
Eric Botcazou

Re: [PATCH] Avoid creating useless debug temporaries

[Jakub Jelinek]

On Tue, Apr 25, 2023 at 05:35:36PM +0200, Eric Botcazou wrote:
> > Haven't looked into detail, but just saving compilation time shouldn't be
> > the only factor when deciding about debug info stuff, another and perhaps
> > even more important would be whether it affects the emitted debug info.
> 
> At least it doesn't change the guality results.

That is too weak test unfortunately.

	Jakub

Re: [PATCH] Avoid creating useless debug temporaries

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 1249 bytes --]

> probably also helps PR109612 and the other similar PR referenced therein.

Here's a more aggressive patch in this area, but it regresses guality tests, 
for example:

+FAIL: gcc.dg/guality/ipa-sra-1.c   -O2  -DPREVENT_OPTIMIZATION  line 27 k == 
3
+FAIL: gcc.dg/guality/ipa-sra-1.c   -O3 -g  -DPREVENT_OPTIMIZATION  line 27 k 
== 3
+FAIL: gcc.dg/guality/ipa-sra-1.c   -Os  -DPREVENT_OPTIMIZATION  line 27 k == 
3

eric@fomalhaut:~/build/gcc/native> diff -u ipa-sra-1.c.254t.optimized.0 ipa-
sra-1.c.254t.optimized
--- ipa-sra-1.c.254t.optimized.0        2023-04-26 11:12:07.806357325 +0200
+++ ipa-sra-1.c.254t.optimized  2023-04-26 11:24:08.632874257 +0200
@@ -101,7 +101,6 @@
   # DEBUG k => k_5
   # DEBUG BEGIN_STMT
   _1 = get_val1 ();
-  # DEBUG D#6 => k_5
   r_8 = foo.isra (_1);
   # DEBUG r => r_8
   # DEBUG BEGIN_STMT

and I don't understand why yet.


	* tree-ssa-dce.cc (find_debug_expr_decl): New callback.
	(mark_stmt_if_obviously_necessary): Add DECLS parameters.
	<GIMPLE_DEBUG>: Call find_debug_expr_decl on the value of
	DEBUG_BIND statements and record the results in DECLS.
	(find_obviously_necessary_stmts): If DEBUG_BIND statements may be
	present, get rid of those setting an unnecessary DEBUG_EXPR_DECL.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 4023 bytes --]

diff --git a/gcc/tree-ssa-dce.cc b/gcc/tree-ssa-dce.cc
index 08876bfc1c7..09bbfaca22e 100644
--- a/gcc/tree-ssa-dce.cc
+++ b/gcc/tree-ssa-dce.cc
@@ -191,14 +191,35 @@ mark_operand_necessary (tree op)
 }
 
 
+/* Called via walk_tree, look for DEBUG_EXPR_DECLs and mark them in DATA.  */
+
+static tree
+find_debug_expr_decl (tree *tp, int *walk_subtrees, void *data)
+{
+  auto_bitmap *decls = (auto_bitmap *) data;
+
+  if (TREE_CODE (*tp) == SSA_NAME || IS_TYPE_OR_DECL_P (*tp))
+    {
+      if (TREE_CODE (*tp) == DEBUG_EXPR_DECL)
+	bitmap_set_bit (*decls, DECL_UID (*tp));
+
+      *walk_subtrees = 0;
+    }
+
+  return NULL_TREE;
+}
+
 /* Mark STMT as necessary if it obviously is.  Add it to the worklist if
    it can make other statements necessary.
 
+   If STMT is a DEBUG_BIND, mark the necessary DEBUG_EXPR_DECLs in DECLS.
+
    If AGGRESSIVE is false, control statements are conservatively marked as
    necessary.  */
 
 static void
-mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
+mark_stmt_if_obviously_necessary (gimple *stmt, auto_bitmap *decls,
+				  bool aggressive)
 {
   /* Statements that are implicitly live.  Most function calls, asm
      and return statements are required.  Labels and GIMPLE_BIND nodes
@@ -258,14 +279,28 @@ mark_stmt_if_obviously_necessary (gimple *stmt, bool aggressive)
       }
 
     case GIMPLE_DEBUG:
-      /* Debug temps without a value are not useful.  ??? If we could
-	 easily locate the debug temp bind stmt for a use thereof,
-	 would could refrain from marking all debug temps here, and
-	 mark them only if they're used.  */
-      if (gimple_debug_nonbind_marker_p (stmt)
-	  || !gimple_debug_bind_p (stmt)
-	  || gimple_debug_bind_has_value_p (stmt)
-	  || TREE_CODE (gimple_debug_bind_get_var (stmt)) != DEBUG_EXPR_DECL)
+      if (gimple_debug_bind_p (stmt))
+	{
+	  tree var = gimple_debug_bind_get_var (stmt);
+	  tree val = gimple_debug_bind_get_value (stmt);
+	  bool necessary = false;
+
+	  /* A bind statement for a real variable is always necessary.  */
+	  if (TREE_CODE (var) != DEBUG_EXPR_DECL)
+	    necessary = true;
+
+	  /* A bind statement with a value is necessary for now and we look
+	     into the value to find out necessary DEBUG_EXPR_DECLs.  */
+	  if (val)
+	    {
+	      walk_tree (&val, find_debug_expr_decl, decls, NULL);
+	      necessary = true;
+	    }
+
+	  if (necessary )
+	    mark_stmt_necessary (stmt, false);
+	}
+      else
 	mark_stmt_necessary (stmt, false);
       return;
 
@@ -398,6 +433,7 @@ find_obviously_necessary_stmts (bool aggressive)
   gimple_stmt_iterator gsi;
   edge e;
   gimple *phi, *stmt;
+  auto_bitmap necessary_decls;
   int flags;
 
   FOR_EACH_BB_FN (bb, cfun)
@@ -414,10 +450,35 @@ find_obviously_necessary_stmts (bool aggressive)
 	{
 	  stmt = gsi_stmt (gsi);
 	  gimple_set_plf (stmt, STMT_NECESSARY, false);
-	  mark_stmt_if_obviously_necessary (stmt, aggressive);
+	  mark_stmt_if_obviously_necessary (stmt, &necessary_decls, aggressive);
 	}
     }
 
+  /* Check all debug bind statements again in the basic blocks and find out
+     those which set an unnecessary DEBUG_EXPR_DECL to a value.  */
+  if (MAY_HAVE_DEBUG_BIND_STMTS)
+    FOR_EACH_BB_FN (bb, cfun)
+      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+	{
+	  stmt = gsi_stmt (gsi);
+	  if (gimple_debug_bind_p (stmt)
+	      && gimple_debug_bind_has_value_p (stmt))
+	    {
+	      tree var = gimple_debug_bind_get_var (stmt);
+	      if (TREE_CODE (var) == DEBUG_EXPR_DECL
+		  && !bitmap_bit_p (necessary_decls, DECL_UID (var)))
+		{
+		  gimple_set_plf (stmt, STMT_NECESSARY, false);
+		  if (dump_file && (dump_flags & TDF_DETAILS))
+		    {
+		      fprintf (dump_file, "Unmarking useful stmt: ");
+		      print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
+		      fprintf (dump_file, "\n");
+		    }
+		}
+	      }
+	  }
+
   /* Pure and const functions are finite and thus have no infinite loops in
      them.  */
   flags = flags_from_decl_or_type (current_function_decl);

Re: [PATCH] Avoid creating useless debug temporaries

[Richard Biener]

On Wed, Apr 26, 2023 at 11:31 AM Eric Botcazou <botcazou@adacore.com> wrote:
>
> > probably also helps PR109612 and the other similar PR referenced therein.
>
> Here's a more aggressive patch in this area, but it regresses guality tests,
> for example:
>
> +FAIL: gcc.dg/guality/ipa-sra-1.c   -O2  -DPREVENT_OPTIMIZATION  line 27 k ==
> 3
> +FAIL: gcc.dg/guality/ipa-sra-1.c   -O3 -g  -DPREVENT_OPTIMIZATION  line 27 k
> == 3
> +FAIL: gcc.dg/guality/ipa-sra-1.c   -Os  -DPREVENT_OPTIMIZATION  line 27 k ==
> 3
>
> eric@fomalhaut:~/build/gcc/native> diff -u ipa-sra-1.c.254t.optimized.0 ipa-
> sra-1.c.254t.optimized
> --- ipa-sra-1.c.254t.optimized.0        2023-04-26 11:12:07.806357325 +0200
> +++ ipa-sra-1.c.254t.optimized  2023-04-26 11:24:08.632874257 +0200
> @@ -101,7 +101,6 @@
>    # DEBUG k => k_5
>    # DEBUG BEGIN_STMT
>    _1 = get_val1 ();
> -  # DEBUG D#6 => k_5
>    r_8 = foo.isra (_1);
>    # DEBUG r => r_8
>    # DEBUG BEGIN_STMT
>
> and I don't understand why yet.

interesting.  So that removes unmentioned debug temporaries?  I think
remove_unused_locals does something to debug stmts as well
(but from a quick look cannot decipher what it actually does).

On the RTL level delete_trivially_dead_insns does wipe some (redundant)
debug_insns, there's no exact match to that on the GIMPLE side either.

I'm not sure if DCE is a good place to do this.

>
>         * tree-ssa-dce.cc (find_debug_expr_decl): New callback.
>         (mark_stmt_if_obviously_necessary): Add DECLS parameters.
>         <GIMPLE_DEBUG>: Call find_debug_expr_decl on the value of
>         DEBUG_BIND statements and record the results in DECLS.
>         (find_obviously_necessary_stmts): If DEBUG_BIND statements may be
>         present, get rid of those setting an unnecessary DEBUG_EXPR_DECL.
>
> --
> Eric Botcazou