diff mbox series

Avoid creating useless debug temporaries

Message ID 3493207.iIbC2pHGDl@fomalhaut
State Accepted
Headers
Series Avoid creating useless debug temporaries |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Eric Botcazou April 25, 2023, 9:33 a.m. UTC 
  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.

Comments

Richard Biener April 25, 2023, 3:10 p.m. UTC | #1 
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
Jakub Jelinek April 25, 2023, 3:20 p.m. UTC | #2 
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
Eric Botcazou April 25, 2023, 3:35 p.m. UTC | #3 
> 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.
Jakub Jelinek April 25, 2023, 3:37 p.m. UTC | #4 
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
Eric Botcazou April 26, 2023, 9:31 a.m. UTC | #5 
> 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.
Richard Biener April 26, 2023, 9:48 a.m. UTC | #6 
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
diff mbox series

Patch

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.  */