[PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Jakub Jelinek]

Hi!

The recent changes to execute_cse_reciprocals cause ICE on the attached
testcase.  The problem is that the first loop over the imm uses ignores
debug stmts (which is correct, whether any debug stmts reference arg1 or not
shouldn't influence the decision on whether to do the transformation or
not), but the second loop assumes all imm uses are assignments with
RDIV_EXPR.  The following patch fixes it by creating a debug temp 1 / arg1
and using it instead of arg1 in the debug stmts.  Alternatively we could
drop the debug stmts, dwarf3/4 isn't able to express floating point
operations anyway (well, in theory it is, but we'd need to implement
floating point arithmetics using integer operations, which would be really
huge).

Ok for trunk if bootstrap/regtest passes?

2009-11-18  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/42078
	* tree-ssa-math-opts.c (execute_cse_reciprocals): Handle
	updating of debug stmt immediate uses.

	* gcc.dg/pr42078.c: New test.

--- gcc/tree-ssa-math-opts.c.jj	2009-11-18 14:56:33.000000000 +0100
+++ gcc/tree-ssa-math-opts.c	2009-11-18 14:27:37.000000000 +0100
@@ -534,6 +534,7 @@ execute_cse_reciprocals (void)
 		  bool md_code, fail;
 		  imm_use_iterator ui;
 		  use_operand_p use_p;
+		  tree debug_expr_decl = NULL_TREE;
 
 		  code = DECL_FUNCTION_CODE (fndecl);
 		  md_code = DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_MD;
@@ -568,6 +569,38 @@ execute_cse_reciprocals (void)
 
 		  FOR_EACH_IMM_USE_STMT (stmt, ui, arg1)
 		    {
+		      if (is_gimple_debug (stmt))
+			{
+			  use_operand_p use_p;
+
+			  if (!debug_expr_decl)
+			    {
+			      gimple def_temp;
+			      tree val;
+			      gimple_stmt_iterator ngsi;
+
+			      debug_expr_decl = make_node (DEBUG_EXPR_DECL);
+			      val = fold_convert (TREE_TYPE (arg1),
+						  integer_one_node);
+			      val = build2 (RDIV_EXPR, TREE_TYPE (arg1),
+					    val, arg1);
+			      def_temp
+				= gimple_build_debug_bind (debug_expr_decl,
+							   val, stmt1);
+			      DECL_ARTIFICIAL (debug_expr_decl) = 1;
+			      TREE_TYPE (debug_expr_decl) = TREE_TYPE (arg1);
+			      DECL_MODE (debug_expr_decl)
+				= TYPE_MODE (TREE_TYPE (arg1));
+
+			      ngsi = gsi_for_stmt (stmt1);
+			      gsi_insert_after (&ngsi, def_temp,
+						GSI_NEW_STMT);
+			    }
+			  FOR_EACH_IMM_USE_ON_STMT (use_p, ui)
+			    SET_USE (use_p, debug_expr_decl);
+			  update_stmt (stmt);
+			  continue;
+			}
 		      gimple_assign_set_rhs_code (stmt, MULT_EXPR);
 		      fold_stmt_inplace (stmt);
 		      update_stmt (stmt);
--- gcc/testsuite/gcc.dg/pr42078.c.jj	2009-11-18 14:58:17.000000000 +0100
+++ gcc/testsuite/gcc.dg/pr42078.c	2009-11-18 14:55:54.000000000 +0100
@@ -0,0 +1,22 @@
+/* PR tree-optimization/42078 */
+/* { dg-do compile } */
+/* { dg-options "-g -O -ffast-math" } */
+
+double sqrt (double x);
+
+float
+foo (float x)
+{
+  float y = sqrt (x);
+  return x / y;
+}
+
+float
+bar (float x)
+{
+  float y = sqrt (x);
+  float a = y;
+  float b = y;
+  float c = y;
+  return x / y;
+}

	Jakub

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Richard Guenther]

On Wed, Nov 18, 2009 at 6:30 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> The recent changes to execute_cse_reciprocals cause ICE on the attached
> testcase.  The problem is that the first loop over the imm uses ignores
> debug stmts (which is correct, whether any debug stmts reference arg1 or not
> shouldn't influence the decision on whether to do the transformation or
> not), but the second loop assumes all imm uses are assignments with
> RDIV_EXPR.  The following patch fixes it by creating a debug temp 1 / arg1
> and using it instead of arg1 in the debug stmts.  Alternatively we could
> drop the debug stmts, dwarf3/4 isn't able to express floating point
> operations anyway (well, in theory it is, but we'd need to implement
> floating point arithmetics using integer operations, which would be really
> huge).
>
> Ok for trunk if bootstrap/regtest passes?

Ugh.  Instead of adding so much code I'd prefer to drop the debug stmt.

Ok with that change.

Thanks,
Richard.

> 2009-11-18  Jakub Jelinek  <jakub@redhat.com>
>
>        PR tree-optimization/42078
>        * tree-ssa-math-opts.c (execute_cse_reciprocals): Handle
>        updating of debug stmt immediate uses.
>
>        * gcc.dg/pr42078.c: New test.
>
> --- gcc/tree-ssa-math-opts.c.jj 2009-11-18 14:56:33.000000000 +0100
> +++ gcc/tree-ssa-math-opts.c    2009-11-18 14:27:37.000000000 +0100
> @@ -534,6 +534,7 @@ execute_cse_reciprocals (void)
>                  bool md_code, fail;
>                  imm_use_iterator ui;
>                  use_operand_p use_p;
> +                 tree debug_expr_decl = NULL_TREE;
>
>                  code = DECL_FUNCTION_CODE (fndecl);
>                  md_code = DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_MD;
> @@ -568,6 +569,38 @@ execute_cse_reciprocals (void)
>
>                  FOR_EACH_IMM_USE_STMT (stmt, ui, arg1)
>                    {
> +                     if (is_gimple_debug (stmt))
> +                       {
> +                         use_operand_p use_p;
> +
> +                         if (!debug_expr_decl)
> +                           {
> +                             gimple def_temp;
> +                             tree val;
> +                             gimple_stmt_iterator ngsi;
> +
> +                             debug_expr_decl = make_node (DEBUG_EXPR_DECL);
> +                             val = fold_convert (TREE_TYPE (arg1),
> +                                                 integer_one_node);
> +                             val = build2 (RDIV_EXPR, TREE_TYPE (arg1),
> +                                           val, arg1);
> +                             def_temp
> +                               = gimple_build_debug_bind (debug_expr_decl,
> +                                                          val, stmt1);
> +                             DECL_ARTIFICIAL (debug_expr_decl) = 1;
> +                             TREE_TYPE (debug_expr_decl) = TREE_TYPE (arg1);
> +                             DECL_MODE (debug_expr_decl)
> +                               = TYPE_MODE (TREE_TYPE (arg1));
> +
> +                             ngsi = gsi_for_stmt (stmt1);
> +                             gsi_insert_after (&ngsi, def_temp,
> +                                               GSI_NEW_STMT);
> +                           }
> +                         FOR_EACH_IMM_USE_ON_STMT (use_p, ui)
> +                           SET_USE (use_p, debug_expr_decl);
> +                         update_stmt (stmt);
> +                         continue;
> +                       }
>                      gimple_assign_set_rhs_code (stmt, MULT_EXPR);
>                      fold_stmt_inplace (stmt);
>                      update_stmt (stmt);
> --- gcc/testsuite/gcc.dg/pr42078.c.jj   2009-11-18 14:58:17.000000000 +0100
> +++ gcc/testsuite/gcc.dg/pr42078.c      2009-11-18 14:55:54.000000000 +0100
> @@ -0,0 +1,22 @@
> +/* PR tree-optimization/42078 */
> +/* { dg-do compile } */
> +/* { dg-options "-g -O -ffast-math" } */
> +
> +double sqrt (double x);
> +
> +float
> +foo (float x)
> +{
> +  float y = sqrt (x);
> +  return x / y;
> +}
> +
> +float
> +bar (float x)
> +{
> +  float y = sqrt (x);
> +  float a = y;
> +  float b = y;
> +  float c = y;
> +  return x / y;
> +}
>
>        Jakub
>

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Alexandre Oliva]

On Nov 18, 2009, Jakub Jelinek <jakub@redhat.com> wrote:

> The recent changes to execute_cse_reciprocals cause ICE on the attached
> testcase.  The problem is that the first loop over the imm uses ignores
> debug stmts (which is correct, whether any debug stmts reference arg1 or not
> shouldn't influence the decision on whether to do the transformation or
> not), but the second loop assumes all imm uses are assignments with
> RDIV_EXPR.

IMHO, the problem is that arg1 is being reused to hold its own
reciprocal.  Should a different assignment of a newly-created SSA_NAME
be created instead, debug bind stmts would be automagically fixed upon
removal or replacement of the original assignment.

Using a different SSA_NAME would also enable the transformation to be
performed even when not all uses can be replaced.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Alexandre Oliva]

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

On Nov 18, 2009, Alexandre Oliva <aoliva@redhat.com> wrote:

> IMHO, the problem is that arg1 is being reused to hold its own
> reciprocal.  Should a different assignment of a newly-created SSA_NAME
> be created instead, debug bind stmts would be automagically fixed upon
> removal or replacement of the original assignment.

I offer two patches below.

They're built upon the understanding that reusing an SSA name's DEF for
a different value is not kosher: it fails to update debug stmts and it
might wreak havoc in any additional side information.

gsi_replace will refuse to replace a stmt with one that sets a different
SSA name, for similar reasons.

The first patch reimplements the code that modified the call stmt in
place, copying it instead, modifying the copy to set a different
variable (just a different SSA name (*), in this case) to the modified
value, inserting the copy before the original, and then removing the
original stmt.

Debug stmts that refer to the removed DEF are reset with the patch
above.  In order to enable the automatic propagation of debug stmts (and
any other code that might be attempting to log or make sense of the
transformations for any other purpose), we have to tell the compiler how
the old DEF relates with the new one before removing the old one.  This
is what the second patch implements.

Is the 1st ok to install?  How about the 2nd, on top of the 1st?


(*) using the same SSA base variable is a bit of a hack.  It might be
easier on the eyes of someone looking at the dumps to actually create a
tmp_var named after “recip” or somesuch.  I couldn't convince myself
this would do us significant good, and this code was slightly shorter,
so I went ahead with it.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-cse-reciprocals-pr42078.patch --]
[-- Type: text/x-diff, Size: 2229 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/42078
	* tree-ssa-math-opts.c (execute_cse_reciprocals): Reset debug
	stmts that refer to the original DEFs.

for  gcc/testsuite/ChangeLog
from  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/42078
	* gcc.dg/pr42078.c: New test.

Index: gcc/tree-ssa-math-opts.c
===================================================================
--- gcc/tree-ssa-math-opts.c.orig	2009-11-18 22:25:06.000000000 -0200
+++ gcc/tree-ssa-math-opts.c	2009-11-19 00:28:05.000000000 -0200
@@ -534,6 +534,9 @@ execute_cse_reciprocals (void)
 		  bool md_code, fail;
 		  imm_use_iterator ui;
 		  use_operand_p use_p;
+		  gimple_stmt_iterator gsi1;
+		  gimple rstmt1;
+		  tree rarg1;
 
 		  code = DECL_FUNCTION_CODE (fndecl);
 		  md_code = DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_MD;
@@ -563,12 +566,22 @@ execute_cse_reciprocals (void)
 		  if (fail)
 		    continue;
 
-		  gimple_call_set_fndecl (stmt1, fndecl);
-		  update_stmt (stmt1);
+		  rstmt1 = gimple_copy (stmt1);
+		  rarg1 = duplicate_ssa_name (arg1, rstmt1);
+		  gimple_call_set_lhs (rstmt1, rarg1);
+		  gimple_call_set_fndecl (rstmt1, fndecl);
+
+		  gsi1 = gsi_for_stmt (stmt1);
+		  gsi_insert_before (&gsi1, rstmt1, true);
+		  /* This will reset debug stmts that refer to arg1,
+		     so we don't have to check for them in the loop
+		     below.  */
+		  gsi_remove (&gsi1, true);
 
 		  FOR_EACH_IMM_USE_STMT (stmt, ui, arg1)
 		    {
 		      gimple_assign_set_rhs_code (stmt, MULT_EXPR);
+		      gimple_assign_set_rhs2 (stmt, rarg1);
 		      fold_stmt_inplace (stmt);
 		      update_stmt (stmt);
 		    }
Index: gcc/testsuite/gcc.dg/pr42078.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.dg/pr42078.c	2009-11-19 00:27:11.000000000 -0200
@@ -0,0 +1,22 @@
+/* PR tree-optimization/42078 */
+/* { dg-do compile } */
+/* { dg-options "-g -O -ffast-math" } */
+
+double sqrt (double x);
+
+float
+foo (float x)
+{
+  float y = sqrt (x);
+  return x / y;
+}
+
+inline float
+bar (float x)
+{
+  float y = sqrt (x);
+  float a = y;
+  float b = y;
+  float c = y;
+  return x / y;
+}

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: vta-cse-reciprocals-preserve-pr42078.patch --]
[-- Type: text/x-diff, Size: 1129 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/42078
	* tree-ssa-math-opts.c (execute_cse_reciprocals): Propagate
	reciprocal-of-reciprocal DEFs into debug stmts.

Index: gcc/tree-ssa-math-opts.c
===================================================================
--- gcc/tree-ssa-math-opts.c.orig	2009-11-19 00:32:12.000000000 -0200
+++ gcc/tree-ssa-math-opts.c	2009-11-19 00:36:56.000000000 -0200
@@ -573,9 +573,19 @@ execute_cse_reciprocals (void)
 
 		  gsi1 = gsi_for_stmt (stmt1);
 		  gsi_insert_before (&gsi1, rstmt1, true);
-		  /* This will reset debug stmts that refer to arg1,
-		     so we don't have to check for them in the loop
-		     below.  */
+
+		  /* This gets debug stmts that refer to arg1 updated
+		     in terms of rarg1.  */
+		  if (MAY_HAVE_DEBUG_STMTS)
+		    {
+		      tree one = build_one_cst (TREE_TYPE (arg1));
+
+		      stmt1 = gimple_build_assign_with_ops (RDIV_EXPR,
+							    arg1,
+							    one,
+							    rarg1);
+		      gsi_replace (&gsi1, stmt1, true);
+		    }
 		  gsi_remove (&gsi1, true);
 
 		  FOR_EACH_IMM_USE_STMT (stmt, ui, arg1)

[-- Attachment #4: Type: text/plain, Size: 258 bytes --]



-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Michael Matz]

Hi,

On Wed, 18 Nov 2009, Alexandre Oliva wrote:

> > The recent changes to execute_cse_reciprocals cause ICE on the 
> > attached testcase.  The problem is that the first loop over the imm 
> > uses ignores debug stmts (which is correct, whether any debug stmts 
> > reference arg1 or not shouldn't influence the decision on whether to 
> > do the transformation or not), but the second loop assumes all imm 
> > uses are assignments with RDIV_EXPR.
> 
> IMHO, the problem is that arg1 is being reused to hold its own 
> reciprocal.  Should a different assignment of a newly-created SSA_NAME 
> be created instead, debug bind stmts would be automagically fixed upon 
> removal or replacement of the original assignment.

Nope, you don't want to leave in the original a/sqrt(b) expression, it 
would result in slower code.  Hence you want to do that transformation 
only when you can replace all uses of the result, meaning that it's not 
necessary to preserve the old SSA name, at which point it's useless 
complexity to generate new instructions and a SSA name.

> Using a different SSA_NAME would also enable the transformation to be 
> performed even when not all uses can be replaced.

This is exactly what you don't want.

Can we drop the debug statement please?


Ciao,
Michael.

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Jakub Jelinek]

On Thu, Nov 19, 2009 at 02:42:24PM +0100, Michael Matz wrote:
> Hi,
> 
> On Wed, 18 Nov 2009, Alexandre Oliva wrote:
> 
> > > The recent changes to execute_cse_reciprocals cause ICE on the 
> > > attached testcase.  The problem is that the first loop over the imm 
> > > uses ignores debug stmts (which is correct, whether any debug stmts 
> > > reference arg1 or not shouldn't influence the decision on whether to 
> > > do the transformation or not), but the second loop assumes all imm 
> > > uses are assignments with RDIV_EXPR.
> > 
> > IMHO, the problem is that arg1 is being reused to hold its own 
> > reciprocal.  Should a different assignment of a newly-created SSA_NAME 
> > be created instead, debug bind stmts would be automagically fixed upon 
> > removal or replacement of the original assignment.
> 
> Nope, you don't want to leave in the original a/sqrt(b) expression, it 
> would result in slower code.  Hence you want to do that transformation 
> only when you can replace all uses of the result, meaning that it's not 
> necessary to preserve the old SSA name, at which point it's useless 
> complexity to generate new instructions and a SSA name.

Please read the patch Alex said, it doesn't leave the original expression in
the IL, but by not doing ugly hacks (and reusing the same SSA name for
something completely different is an ugly hack) you get all the needed debug
info updates for free.  I like Alex' patch, it is much nicer than what I
posted.

	Jakub

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Michael Matz]

Hi,

On Thu, 19 Nov 2009, Jakub Jelinek wrote:

> > Nope, you don't want to leave in the original a/sqrt(b) expression, it 
> > would result in slower code.  Hence you want to do that transformation 
> > only when you can replace all uses of the result, meaning that it's 
> > not necessary to preserve the old SSA name, at which point it's 
> > useless complexity to generate new instructions and a SSA name.
> 
> Please read the patch Alex said, it doesn't leave the original 
> expression in the IL, but by not doing ugly hacks (and reusing the same 
> SSA name for something completely different is an ugly hack) you get all 
> the needed debug info updates for free.  I like Alex' patch, it is much 
> nicer than what I posted.

It is nicer, yes.  But:

+                 rstmt1 = gimple_copy (stmt1);
+                 rarg1 = duplicate_ssa_name (arg1, rstmt1);
+                 gimple_call_set_lhs (rstmt1, rarg1);
+                 gimple_call_set_fndecl (rstmt1, fndecl);
+
+                 gsi1 = gsi_for_stmt (stmt1);
+                 gsi_insert_before (&gsi1, rstmt1, true);
+                 /* This will reset debug stmts that refer to arg1,
+                    so we don't have to check for them in the loop
+                    below.  */
+                 gsi_remove (&gsi1, true);

Instead of copying the statement, inserting it and removing the original, 
it should be possible to simply replace the LHS with a new SSA name (as 
well as the fndecl), shouldn't it?


Ciao,
Michael.

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Richard Guenther]

On Thu, Nov 19, 2009 at 3:47 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Nov 18, 2009, Alexandre Oliva <aoliva@redhat.com> wrote:
>
>> IMHO, the problem is that arg1 is being reused to hold its own
>> reciprocal.  Should a different assignment of a newly-created SSA_NAME
>> be created instead, debug bind stmts would be automagically fixed upon
>> removal or replacement of the original assignment.
>
> I offer two patches below.
>
> They're built upon the understanding that reusing an SSA name's DEF for
> a different value is not kosher: it fails to update debug stmts and it
> might wreak havoc in any additional side information.
>
> gsi_replace will refuse to replace a stmt with one that sets a different
> SSA name, for similar reasons.
>
> The first patch reimplements the code that modified the call stmt in
> place, copying it instead, modifying the copy to set a different
> variable (just a different SSA name (*), in this case) to the modified
> value, inserting the copy before the original, and then removing the
> original stmt.
>
> Debug stmts that refer to the removed DEF are reset with the patch
> above.  In order to enable the automatic propagation of debug stmts (and
> any other code that might be attempting to log or make sense of the
> transformations for any other purpose), we have to tell the compiler how
> the old DEF relates with the new one before removing the old one.  This
> is what the second patch implements.
>
> Is the 1st ok to install?  How about the 2nd, on top of the 1st?

I don't like the 2nd patch too much.  As of the first one I agree
with the rationale but the implementation looks ugly (and
at least would need a comment).

Wouldn't

   rarg1 = make_ssa_name (SSA_NAME_VAR (gimple_call_lhs (stmt1)), stmt1);
   release_defs (stmt1);
   gimple_call_set_lhs (stmt1, rarg1);
   gimple_call_set_fndecl (stmt1);
   update_stmt (stmt1);

work as well?  That avoids the stmt duplication.

In fact this exchanging of the LHS (or rather invalidating of the
SSA name value) should be a helper function that knows
the implementation details and avoids going through releasing
and allocating the name.

Richard.

>
> (*) using the same SSA base variable is a bit of a hack.  It might be
> easier on the eyes of someone looking at the dumps to actually create a
> tmp_var named after “recip” or somesuch.  I couldn't convince myself
> this would do us significant good, and this code was slightly shorter,
> so I went ahead with it.
>
>
>
>
> --
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist      Red Hat Brazil Compiler Engineer
>
>

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Alexandre Oliva]

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

On Nov 19, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:

> In fact this exchanging of the LHS (or rather invalidating of the
> SSA name value) should be a helper function that knows
> the implementation details and avoids going through releasing
> and allocating the name.

Okie dokie, here's a sequence of patches that implements helper
functions for this sort of stuff.

The first patch introduces machinery to propagate “dying” DEFs into
debug stmts, while replacing them with other SSA_NAMEs.

The second extends it so as to enable the old LHS to be redefined
e.g. in terms of the new LHS.  IIRC this may be useful in some other
transformations that, in the process of introducing VTA, I changed from
modifying stmts in place to inserting new stmts and removing others.  I
haven't looked for them yet.

The third uses this new feature for the case at hand, while avoiding
computing the reciprocal expression if we know it won't be used.

Are any of them ok to install, if they pass regstrap?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-cse-reciprocals-pr42078.patch --]
[-- Type: text/x-diff, Size: 3518 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/42078
	* gimple.h (gimple_replace_lhs): New declaration.
	* gimple.c (gimple_replace_lhs): New function.
	* tree-ssa-math-opts.c (execute_cse_reciprocals): Call it before
	modifying the call.

for  gcc/testsuite/ChangeLog
from  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/42078
	* gcc.dg/pr42078.c: New test.

Index: gcc/gimple.h
===================================================================
--- gcc/gimple.h.orig	2009-11-20 06:15:41.000000000 -0200
+++ gcc/gimple.h	2009-11-20 06:18:14.000000000 -0200
@@ -843,6 +843,7 @@ void gimple_assign_set_rhs_with_ops (gim
 				     tree, tree);
 tree gimple_get_lhs (const_gimple);
 void gimple_set_lhs (gimple, tree);
+void gimple_replace_lhs (gimple, tree);
 gimple gimple_copy (gimple);
 bool is_gimple_operand (const_tree);
 void gimple_set_modified (gimple, bool);
Index: gcc/gimple.c
===================================================================
--- gcc/gimple.c.orig	2009-11-20 06:15:42.000000000 -0200
+++ gcc/gimple.c	2009-11-20 06:18:25.000000000 -0200
@@ -1981,6 +1981,38 @@ gimple_set_lhs (gimple stmt, tree lhs)
     gcc_unreachable();
 }
 
+/* Replace the LHS of STMT, an assignment, either a GIMPLE_ASSIGN or a
+   GIMPLE_CALL, with NLHS, in preparation for modifying the RHS to an
+   expression with a different value.
+
+   This will update any annotations (say debug bind stmts) referring
+   to the original LHS, so that they use the RHS instead.  This is
+   done even if NLHS and LHS are the same, for it is understood that
+   the RHS will be modified afterwards, and NLHS will not be assigned
+   an equivalent value.
+
+   Adjusting any non-annotation uses of the LHS, if needed, is a
+   responsibility of the caller.
+
+   The effect of this call should be pretty much the same as that of
+   inserting a copy of STMT before STMT, and then removing the
+   original stmt, at which time gsi_remove() would have update
+   annotations, but using this function saves all the inserting,
+   copying and removing.  */
+
+void
+gimple_replace_lhs (gimple stmt, tree nlhs)
+{
+  if (MAY_HAVE_DEBUG_STMTS)
+    {
+      gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
+      tree lhs = gimple_get_lhs (stmt);
+
+      insert_debug_temp_for_var_def (&gsi, lhs);
+    }
+
+  gimple_set_lhs (stmt, nlhs);
+}
 
 /* Return a deep copy of statement STMT.  All the operands from STMT
    are reallocated and copied using unshare_expr.  The DEF, USE, VDEF
Index: gcc/tree-ssa-math-opts.c
===================================================================
--- gcc/tree-ssa-math-opts.c.orig	2009-11-20 06:15:41.000000000 -0200
+++ gcc/tree-ssa-math-opts.c	2009-11-20 06:18:14.000000000 -0200
@@ -563,6 +563,7 @@ execute_cse_reciprocals (void)
 		  if (fail)
 		    continue;
 
+		  gimple_replace_lhs (stmt1, arg1);
 		  gimple_call_set_fndecl (stmt1, fndecl);
 		  update_stmt (stmt1);
 
Index: gcc/testsuite/gcc.dg/pr42078.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.dg/pr42078.c	2009-11-20 06:16:30.000000000 -0200
@@ -0,0 +1,22 @@
+/* PR tree-optimization/42078 */
+/* { dg-do compile } */
+/* { dg-options "-g -O -ffast-math" } */
+
+double sqrt (double x);
+
+float
+foo (float x)
+{
+  float y = sqrt (x);
+  return x / y;
+}
+
+inline float
+bar (float x)
+{
+  float y = sqrt (x);
+  float a = y;
+  float b = y;
+  float c = y;
+  return x / y;
+}

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: vta-debug-temps-explicit-values-pr42078.patch --]
[-- Type: text/x-diff, Size: 8783 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/42078
	* gimple.h (gimple_replace_lhs_wants_value): New.
	(gimple_replace_lhs): Add value.
	* gimple.c (gimple_replace_lhs): Likewise.  Pass it on.
	* tree-flow.h (insert_debug_temp_for_var_def): Add value.
	* tree-ssa.c (insert_debug_temp_for_var_def): Likewise.  Use it.
	(insert_debug_temps_for_defs): Pass NULL value.
	* tree-ssanames.c (release_ssa_name): Likewise.
	* tree-ssa-math-opts.c (execute_cse_reciprocals): Likewise.

Index: gcc/gimple.h
===================================================================
--- gcc/gimple.h.orig	2009-11-20 06:18:14.000000000 -0200
+++ gcc/gimple.h	2009-11-20 06:18:35.000000000 -0200
@@ -843,7 +843,7 @@ void gimple_assign_set_rhs_with_ops (gim
 				     tree, tree);
 tree gimple_get_lhs (const_gimple);
 void gimple_set_lhs (gimple, tree);
-void gimple_replace_lhs (gimple, tree);
+void gimple_replace_lhs (gimple, tree, tree);
 gimple gimple_copy (gimple);
 bool is_gimple_operand (const_tree);
 void gimple_set_modified (gimple, bool);
@@ -2226,6 +2226,14 @@ gimple_has_lhs (gimple stmt)
 	      && gimple_call_lhs (stmt) != NULL_TREE));
 }
 
+/* Return true if it might be useful to pass a VALUE to
+   gimple_replace_lhs ().  */
+static inline bool
+gimple_replace_lhs_wants_value (void)
+{
+  return MAY_HAVE_DEBUG_STMTS;
+}
+
 
 /* Return the code of the predicate computed by conditional statement GS.  */
 
Index: gcc/gimple.c
===================================================================
--- gcc/gimple.c.orig	2009-11-20 06:18:25.000000000 -0200
+++ gcc/gimple.c	2009-11-20 06:20:35.000000000 -0200
@@ -1986,10 +1986,10 @@ gimple_set_lhs (gimple stmt, tree lhs)
    expression with a different value.
 
    This will update any annotations (say debug bind stmts) referring
-   to the original LHS, so that they use the RHS instead.  This is
-   done even if NLHS and LHS are the same, for it is understood that
-   the RHS will be modified afterwards, and NLHS will not be assigned
-   an equivalent value.
+   to the original LHS, so that they use VALUE or the RHS instead.
+   This is done even if NLHS and LHS are the same, for it is
+   understood that the RHS will be modified afterwards, and NLHS will
+   not be assigned an equivalent value.
 
    Adjusting any non-annotation uses of the LHS, if needed, is a
    responsibility of the caller.
@@ -2001,14 +2001,19 @@ gimple_set_lhs (gimple stmt, tree lhs)
    copying and removing.  */
 
 void
-gimple_replace_lhs (gimple stmt, tree nlhs)
+gimple_replace_lhs (gimple stmt, tree nlhs, tree value)
 {
+  /* If the conditions in which this function uses VALUE change,
+     adjust gimple_replace_lhs_wants_value().  */
+  gcc_assert (gimple_replace_lhs_wants_value ()
+	      == MAY_HAVE_DEBUG_STMTS);
+
   if (MAY_HAVE_DEBUG_STMTS)
     {
       gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
       tree lhs = gimple_get_lhs (stmt);
 
-      insert_debug_temp_for_var_def (&gsi, lhs);
+      insert_debug_temp_for_var_def (&gsi, lhs, value);
     }
 
   gimple_set_lhs (stmt, nlhs);
Index: gcc/tree-flow.h
===================================================================
--- gcc/tree-flow.h.orig	2009-11-20 06:18:15.000000000 -0200
+++ gcc/tree-flow.h	2009-11-20 06:18:35.000000000 -0200
@@ -638,7 +638,7 @@ typedef bool (*walk_use_def_chains_fn) (
 extern void walk_use_def_chains (tree, walk_use_def_chains_fn, void *, bool);
 
 void insert_debug_temps_for_defs (gimple_stmt_iterator *);
-void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree);
+void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree, tree);
 void release_defs_bitset (bitmap toremove);
 
 /* In tree-into-ssa.c  */
Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c.orig	2009-11-20 06:18:14.000000000 -0200
+++ gcc/tree-ssa.c	2009-11-20 07:03:50.000000000 -0200
@@ -297,17 +297,25 @@ find_released_ssa_name (tree *tp, int *w
 
 /* Insert a DEBUG BIND stmt before the DEF of VAR if VAR is referenced
    by other DEBUG stmts, and replace uses of the DEF with the
-   newly-created debug temp.  */
+   newly-created debug temp, or with the DEF's value, if the
+   substitution is valid.  The value is taken from VALUE, if given, or
+   from the DEF stmt, taken from GSI, if given, or from the DEF of
+   VAR.  If VALUE and VAR are the same, no DEBUG BIND stmt will be
+   created, and all uses of VAR will be reset.  If a DEBUG BIND stmt
+   has to be created, it will be inserted before or after GSI or the
+   DEF, depending */
 
 void
-insert_debug_temp_for_var_def (gimple_stmt_iterator *gsi, tree var)
+insert_debug_temp_for_var_def (gimple_stmt_iterator *gsi, tree var, tree value)
 {
   imm_use_iterator imm_iter;
   use_operand_p use_p;
   gimple stmt;
   gimple def_stmt = NULL;
   int usecount = 0;
-  tree value = NULL;
+  bool given_value;
+  gimple def_temp = NULL;
+  gimple_stmt_iterator ngsi;
 
   if (!MAY_HAVE_DEBUG_STMTS)
     return;
@@ -343,10 +351,13 @@ insert_debug_temp_for_var_def (gimple_st
   else
     def_stmt = SSA_NAME_DEF_STMT (var);
 
+  given_value = false;
+  if (value)
+    given_value = true;
   /* If we didn't get an insertion point, and the stmt has already
      been removed, we won't be able to insert the debug bind stmt, so
      we'll have to drop debug information.  */
-  if (gimple_code (def_stmt) == GIMPLE_PHI)
+  else if (gimple_code (def_stmt) == GIMPLE_PHI)
     {
       value = degenerate_phi_result (def_stmt);
       if (value && walk_tree (&value, find_released_ssa_name, NULL, NULL))
@@ -397,6 +408,9 @@ insert_debug_temp_for_var_def (gimple_st
 	value = gimple_assign_rhs_to_tree (def_stmt);
     }
 
+  if (value == var)
+    value = NULL;
+
   if (value)
     {
       /* If there's a single use of VAR, and VAR is the entire debug
@@ -414,7 +428,7 @@ insert_debug_temp_for_var_def (gimple_st
 	 at the expense of duplication of expressions.  */
 
       if (CONSTANT_CLASS_P (value)
-	  || gimple_code (def_stmt) == GIMPLE_PHI
+	  || (gimple_code (def_stmt) == GIMPLE_PHI && !given_value)
 	  || (usecount == 1
 	      && (!gimple_assign_single_p (def_stmt)
 		  || is_gimple_min_invariant (value)))
@@ -422,7 +436,6 @@ insert_debug_temp_for_var_def (gimple_st
 	value = unshare_expr (value);
       else
 	{
-	  gimple def_temp;
 	  tree vexpr = make_node (DEBUG_EXPR_DECL);
 
 	  def_temp = gimple_build_debug_bind (vexpr,
@@ -436,14 +449,27 @@ insert_debug_temp_for_var_def (gimple_st
 	  else
 	    DECL_MODE (vexpr) = TYPE_MODE (TREE_TYPE (value));
 
-	  if (gsi)
-	    gsi_insert_before (gsi, def_temp, GSI_SAME_STMT);
-	  else
+	  if (gimple_code (def_stmt) == GIMPLE_PHI)
+	    {
+	      basic_block bb;
+
+	      if (gsi)
+		bb = gsi_bb (*gsi);
+	      else
+		bb = gimple_bb (def_stmt);
+
+	      ngsi = gsi_after_labels (bb);
+	      gsi = &ngsi;
+	    }
+	  else if (!gsi)
 	    {
-	      gimple_stmt_iterator ngsi = gsi_for_stmt (def_stmt);
-	      gsi_insert_before (&ngsi, def_temp, GSI_SAME_STMT);
+	      ngsi = gsi_for_stmt (def_stmt);
+	      gsi = &ngsi;
 	    }
 
+	  if (!given_value)
+	    gsi_insert_before (gsi, def_temp, GSI_SAME_STMT);
+
 	  value = vexpr;
 	}
     }
@@ -466,6 +492,11 @@ insert_debug_temp_for_var_def (gimple_st
 
       update_stmt (stmt);
     }
+
+  /* Insert the definition last, so that we don't modify it in case
+     VALUE references VAR.  Assume the caller intends it to be so.  */
+  if (def_temp && given_value)
+    gsi_insert_after (gsi, def_temp, GSI_SAME_STMT);
 }
 
 
@@ -492,7 +523,7 @@ insert_debug_temps_for_defs (gimple_stmt
       if (TREE_CODE (var) != SSA_NAME)
 	continue;
 
-      insert_debug_temp_for_var_def (gsi, var);
+      insert_debug_temp_for_var_def (gsi, var, NULL);
     }
 }
 
Index: gcc/tree-ssanames.c
===================================================================
--- gcc/tree-ssanames.c.orig	2009-11-20 06:18:14.000000000 -0200
+++ gcc/tree-ssanames.c	2009-11-20 06:18:35.000000000 -0200
@@ -206,7 +206,7 @@ release_ssa_name (tree var)
       use_operand_p imm = &(SSA_NAME_IMM_USE_NODE (var));
 
       if (MAY_HAVE_DEBUG_STMTS)
-	insert_debug_temp_for_var_def (NULL, var);
+	insert_debug_temp_for_var_def (NULL, var, NULL);
 
 #ifdef ENABLE_CHECKING
       verify_imm_links (stderr, var);
Index: gcc/tree-ssa-math-opts.c
===================================================================
--- gcc/tree-ssa-math-opts.c.orig	2009-11-20 06:18:14.000000000 -0200
+++ gcc/tree-ssa-math-opts.c	2009-11-20 07:03:50.000000000 -0200
@@ -563,7 +563,7 @@ execute_cse_reciprocals (void)
 		  if (fail)
 		    continue;
 
-		  gimple_replace_lhs (stmt1, arg1);
+		  gimple_replace_lhs (stmt1, arg1, NULL);
 		  gimple_call_set_fndecl (stmt1, fndecl);
 		  update_stmt (stmt1);
 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: vta-cse-reciprocals-preserve-pr42078.patch --]
[-- Type: text/x-diff, Size: 1343 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/42078
	* tree-ssa-math-opts.c (execute_cse_reciprocals): Compute reciprocal
	value for debug stmts.  

Index: gcc/tree-ssa-math-opts.c
===================================================================
--- gcc/tree-ssa-math-opts.c.orig	2009-11-20 06:43:14.000000000 -0200
+++ gcc/tree-ssa-math-opts.c	2009-11-20 06:43:19.000000000 -0200
@@ -534,6 +534,7 @@ execute_cse_reciprocals (void)
 		  bool md_code, fail;
 		  imm_use_iterator ui;
 		  use_operand_p use_p;
+		  tree value;
 
 		  code = DECL_FUNCTION_CODE (fndecl);
 		  md_code = DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_MD;
@@ -563,12 +564,23 @@ execute_cse_reciprocals (void)
 		  if (fail)
 		    continue;
 
-		  gimple_replace_lhs (stmt1, arg1, NULL);
+		  if (gimple_replace_lhs_wants_value ())
+		    {
+		      tree t = TREE_TYPE (arg1);
+
+		      value = build2 (RDIV_EXPR, t, build_one_cst (t), arg1);
+		    }
+		  else
+		    value = NULL;
+
+		  gimple_replace_lhs (stmt1, arg1, value);
 		  gimple_call_set_fndecl (stmt1, fndecl);
 		  update_stmt (stmt1);
 
 		  FOR_EACH_IMM_USE_STMT (stmt, ui, arg1)
 		    {
+		      if (is_gimple_debug (stmt))
+			continue;
 		      gimple_assign_set_rhs_code (stmt, MULT_EXPR);
 		      fold_stmt_inplace (stmt);
 		      update_stmt (stmt);

[-- Attachment #5: Type: text/plain, Size: 257 bytes --]


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Richard Guenther]

On Fri, Nov 20, 2009 at 10:36 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Nov 19, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> In fact this exchanging of the LHS (or rather invalidating of the
>> SSA name value) should be a helper function that knows
>> the implementation details and avoids going through releasing
>> and allocating the name.
>
> Okie dokie, here's a sequence of patches that implements helper
> functions for this sort of stuff.
>
> The first patch introduces machinery to propagate “dying” DEFs into
> debug stmts, while replacing them with other SSA_NAMEs.
>
> The second extends it so as to enable the old LHS to be redefined
> e.g. in terms of the new LHS.  IIRC this may be useful in some other
> transformations that, in the process of introducing VTA, I changed from
> modifying stmts in place to inserting new stmts and removing others.  I
> haven't looked for them yet.
>
> The third uses this new feature for the case at hand, while avoiding
> computing the reciprocal expression if we know it won't be used.
>
> Are any of them ok to install, if they pass regstrap?

The vta-cse-reciprocals-pr42078.patch patch is ok.

I think the
other twos are overzealous at the moment, at least I don't see
why gimple_replace_lhs couldn't get the value on its own.  That is,
I probably misunderstand how it works and thus the stuff is
somewhat confusing.

Thanks,
Richard.

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Paolo Bonzini]

On 11/20/2009 10:36 AM, Alexandre Oliva wrote:
> This will update any annotations (say debug bind stmts) referring
> to the original LHS, so that they use the RHS instead

"... so that they use a debug temporary instead".

It took me a while to understand this sentence.

Paolo

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Alexandre Oliva]

On Nov 20, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:

> I don't see why gimple_replace_lhs couldn't get the value on its own.

It can, but that's not necessarily the best representation for the
value.

Consider the case at hand.  We have say

  y_2 = builtin_sqrt(x_1)
  # DEBUG y => y_2

now, we want to effectively drop this y_2 and introduce another
expression that evaluates to the reciprocal.  I.e.:

  reciprocal_of_y_2 = builtin_rsqrt(x_1)
  
but if we take the expression originally stored in y_2, which is the
best gimple_replace_lhs could do, we'll end up with:

  # DEBUG y => builtin_sqrt(x_1)

while is both non-gimple and suboptimal, given that now there's a much
simpler way to compute the value of y, namely:

  # DEBUG y => 1.0 / reciprocal_of_y_2

It could be argued that gimple_replace_lhs or insert_debug_temps or
somesuch could be given enough brains to figure this out, but they would
then have to be told what the new lhs is going to be set to (let's say
newval), and try to figure out how to get from newval to the current rhs
(say oldval).  That's unnecessary complexity, given that the caller
knows exactly how one relates to the other, and it must know it.

This new interface provides us with a way to tell the debug temps
machinery how the released DEF relates with other DEFs that remain or
that will be introduced.

Think how useful this could be, for example, for IV elimination.

Given:

  sum = 0;
  for (i = 0; i < n; i++)
    sum += a[i];
  
i.e. (in a representation that exposed the address computations in array
indexing):

 <entry>:
  sum_3 = 0;
  # DEBUG sum => sum_3
  i_1 = 0;
  # DEBUG i => i_1
  goto <test>;

 <body>:
  T_10 = i_6 * <sizeof(*a)>;
  T_9 = a + T_10;
  T_8 = *T_9;
  sum_5 = sum_4 + T_8;
  # DEBUG sum => sum_5
  i_7 = i_6 + 1;
  # DEBUG i => i_7

 <test>:
  sum_4 = PHI <sum_3(<entry>), sum_5(<body>)>;
  i_6 = PHI <i_1(<entry>), i_7(<body>)>;
  # DEBUG sum => sum_4
  # DEBUG i => i_6
  if (i_6 < n_2(D))
    goto <body>;

Now, if we were to perform strength reduction:

 <entry>:
  sum_3 = 0;
  # DEBUG sum => sum_3
  ia_11 = a;
  T_12 = n_2(D) * sizeof(*a);
  ea_13 = ia_11 + T_12;
  # DEBUG i => 0
  goto <test>;

 <body>:
  T_8 = *ia_14;
  sum_5 = sum_4 + T_8;
  # DEBUG sum => sum_5
  ia_15 = ia_14 + sizeof(*a);
  # DEBUG i => NULL

 <test>:
  sum_4 = PHI <sum_3(<entry>), sum_5(<body>)>;
  ia_14 = PHI <ia_11(<entry>), ia_15(<body>)>;
  # DEBUG sum => sum_4
  # DEBUG i => NULL
  if (ia_14 < ea_13)
    goto <body>;

Oops, i is now completely gone.  Although i_7 was propagated into the
debug bind stmt in the loop body, when the PHI node was deleted there
was nothing we could do to preserve it.

We need some way to tell how to compute IVs that are removed based on
other IVs that remain, and this machinery would enable us to do just
that.  E.g., since we know that:

  ia = a + i * sizeof(*a);

we might very well compute i:

  i = (ia - a) / sizeof(*a);

and use this expression *instead* of dumbly failing to propagate the
increments into debug stmts.  Then we'd have:

  # DEBUG i => (ia_15 - ia_11) / sizeof(*a)

within the loop body.

Handling the confluence in the loop exit test would require implementing
one of the ??? in insert_debug_temps, namely inserting debug temps at
the end of incoming edges, yielding something like:

 <entry>:
  sum_3 = 0;
  # DEBUG sum => sum_3
  ia_11 = a;
  T_12 = n_2(D) * sizeof(*a);
  ea_13 = ia_11 + T_12;
  # DEBUG D#1 => 0
  # DEBUG i => D#1
  goto <test>;

 <body>:
  T_8 = *ia_14;
  sum_5 = sum_4 + T_8;
  # DEBUG sum => sum_5
  ia_15 = ia_14 + sizeof(*a);
  # DEBUG D#1 => (ia_15 - ia_11) / sizeof(*a)
  # DEBUG i => D#1

 <test>:
  sum_4 = PHI <sum_3(<entry>), sum_5(<body>)>;
  ia_14 = PHI <ia_11(<entry>), ia_15(<body>)>;
  # DEBUG sum => sum_4
  # DEBUG i => D#1
  if (ia_14 < ea_13)
    goto <body>;

I don't see how insert_debug_temps could make this kind of inference.


Further consider rewrite_bittest() in tree-ssa-loop-im.c.  Given:

  _3 = A_1 >> B_2;
  _4 = _3 & 1;
  # DEBUG foo => _4
  if (_4 [=!]= 0)
    ...

it optimizes to the following, in which _5 can be hoisted out of the
loop:

  # DEBUG D#1 (A_1 >> B_2)
  _5 = 1 << B_2;
  _6 = A_1 & _5;
  # DEBUG foo => D#1 & 1
  if (_6 [=!]= 0)
    ...
  
Now, if we could somehow communicate the debug machinery that _4 can be
computed from the new defs, we'd end up with fewer and simpler debug
stmts and temps:

  _5 = 1 << B_2;
  _6 = A_1 & _5;
  # DEBUG foo => _6 >> B_2
  if (_6 [=!]= 0)

I suspect other changes that went in in the initial debug temps patch
could benefit similarly:

	* tree-ssa-reassoc.c (rewrite_expr_tree, linearize_expr): Likewise.
	* tree-ssa-sink.c (statement_sink_location): Likewise.
	* tree-ssa-forwprop (forward_propagate_addr_expr): Likewise.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Richard Guenther]

On Sat, Nov 21, 2009 at 7:03 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Nov 20, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> I don't see why gimple_replace_lhs couldn't get the value on its own.
>
> It can, but that's not necessarily the best representation for the
> value.
>
> Consider the case at hand.  We have say
>
>  y_2 = builtin_sqrt(x_1)
>  # DEBUG y => y_2
>
> now, we want to effectively drop this y_2 and introduce another
> expression that evaluates to the reciprocal.  I.e.:
>
>  reciprocal_of_y_2 = builtin_rsqrt(x_1)
>
> but if we take the expression originally stored in y_2, which is the
> best gimple_replace_lhs could do, we'll end up with:
>
>  # DEBUG y => builtin_sqrt(x_1)
>
> while is both non-gimple and suboptimal, given that now there's a much
> simpler way to compute the value of y, namely:
>
>  # DEBUG y => 1.0 / reciprocal_of_y_2
>
> It could be argued that gimple_replace_lhs or insert_debug_temps or
> somesuch could be given enough brains to figure this out, but they would
> then have to be told what the new lhs is going to be set to (let's say
> newval), and try to figure out how to get from newval to the current rhs
> (say oldval).  That's unnecessary complexity, given that the caller
> knows exactly how one relates to the other, and it must know it.
>
> This new interface provides us with a way to tell the debug temps
> machinery how the released DEF relates with other DEFs that remain or
> that will be introduced.
>
> Think how useful this could be, for example, for IV elimination.
>
> Given:
>
>  sum = 0;
>  for (i = 0; i < n; i++)
>    sum += a[i];
>
> i.e. (in a representation that exposed the address computations in array
> indexing):
>
>  <entry>:
>  sum_3 = 0;
>  # DEBUG sum => sum_3
>  i_1 = 0;
>  # DEBUG i => i_1
>  goto <test>;
>
>  <body>:
>  T_10 = i_6 * <sizeof(*a)>;
>  T_9 = a + T_10;
>  T_8 = *T_9;
>  sum_5 = sum_4 + T_8;
>  # DEBUG sum => sum_5
>  i_7 = i_6 + 1;
>  # DEBUG i => i_7
>
>  <test>:
>  sum_4 = PHI <sum_3(<entry>), sum_5(<body>)>;
>  i_6 = PHI <i_1(<entry>), i_7(<body>)>;
>  # DEBUG sum => sum_4
>  # DEBUG i => i_6
>  if (i_6 < n_2(D))
>    goto <body>;
>
> Now, if we were to perform strength reduction:
>
>  <entry>:
>  sum_3 = 0;
>  # DEBUG sum => sum_3
>  ia_11 = a;
>  T_12 = n_2(D) * sizeof(*a);
>  ea_13 = ia_11 + T_12;
>  # DEBUG i => 0
>  goto <test>;
>
>  <body>:
>  T_8 = *ia_14;
>  sum_5 = sum_4 + T_8;
>  # DEBUG sum => sum_5
>  ia_15 = ia_14 + sizeof(*a);
>  # DEBUG i => NULL
>
>  <test>:
>  sum_4 = PHI <sum_3(<entry>), sum_5(<body>)>;
>  ia_14 = PHI <ia_11(<entry>), ia_15(<body>)>;
>  # DEBUG sum => sum_4
>  # DEBUG i => NULL
>  if (ia_14 < ea_13)
>    goto <body>;
>
> Oops, i is now completely gone.  Although i_7 was propagated into the
> debug bind stmt in the loop body, when the PHI node was deleted there
> was nothing we could do to preserve it.
>
> We need some way to tell how to compute IVs that are removed based on
> other IVs that remain, and this machinery would enable us to do just
> that.  E.g., since we know that:
>
>  ia = a + i * sizeof(*a);
>
> we might very well compute i:
>
>  i = (ia - a) / sizeof(*a);
>
> and use this expression *instead* of dumbly failing to propagate the
> increments into debug stmts.  Then we'd have:
>
>  # DEBUG i => (ia_15 - ia_11) / sizeof(*a)
>
> within the loop body.
>
> Handling the confluence in the loop exit test would require implementing
> one of the ??? in insert_debug_temps, namely inserting debug temps at
> the end of incoming edges, yielding something like:
>
>  <entry>:
>  sum_3 = 0;
>  # DEBUG sum => sum_3
>  ia_11 = a;
>  T_12 = n_2(D) * sizeof(*a);
>  ea_13 = ia_11 + T_12;
>  # DEBUG D#1 => 0
>  # DEBUG i => D#1
>  goto <test>;
>
>  <body>:
>  T_8 = *ia_14;
>  sum_5 = sum_4 + T_8;
>  # DEBUG sum => sum_5
>  ia_15 = ia_14 + sizeof(*a);
>  # DEBUG D#1 => (ia_15 - ia_11) / sizeof(*a)
>  # DEBUG i => D#1
>
>  <test>:
>  sum_4 = PHI <sum_3(<entry>), sum_5(<body>)>;
>  ia_14 = PHI <ia_11(<entry>), ia_15(<body>)>;
>  # DEBUG sum => sum_4
>  # DEBUG i => D#1
>  if (ia_14 < ea_13)
>    goto <body>;
>
> I don't see how insert_debug_temps could make this kind of inference.
>
>
> Further consider rewrite_bittest() in tree-ssa-loop-im.c.  Given:
>
>  _3 = A_1 >> B_2;
>  _4 = _3 & 1;
>  # DEBUG foo => _4
>  if (_4 [=!]= 0)
>    ...
>
> it optimizes to the following, in which _5 can be hoisted out of the
> loop:
>
>  # DEBUG D#1 (A_1 >> B_2)
>  _5 = 1 << B_2;
>  _6 = A_1 & _5;
>  # DEBUG foo => D#1 & 1
>  if (_6 [=!]= 0)
>    ...
>
> Now, if we could somehow communicate the debug machinery that _4 can be
> computed from the new defs, we'd end up with fewer and simpler debug
> stmts and temps:
>
>  _5 = 1 << B_2;
>  _6 = A_1 & _5;
>  # DEBUG foo => _6 >> B_2
>  if (_6 [=!]= 0)
>
> I suspect other changes that went in in the initial debug temps patch
> could benefit similarly:
>
>        * tree-ssa-reassoc.c (rewrite_expr_tree, linearize_expr): Likewise.
>        * tree-ssa-sink.c (statement_sink_location): Likewise.
>        * tree-ssa-forwprop (forward_propagate_addr_expr): Likewise.

I see.  Definitely something that we should investigate during stage1.
At least the code needs more comments - otherwise I could have figured
out the above myself ;)

Richard.

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Alexandre Oliva]

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

According to http://gcc.gnu.org/ml/gcc-patches/2009-11/msg01082.html
on Nov 20, 2009, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Nov 19, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>> In fact this exchanging of the LHS (or rather invalidating of the
>> SSA name value) should be a helper function that knows
>> the implementation details and avoids going through releasing
>> and allocating the name.

> Okie dokie, here's a sequence of patches that implements helper
> functions for this sort of stuff.

> The first patch introduces machinery to propagate “dying” DEFs into
> debug stmts, while replacing them with other SSA_NAMEs.

This is already in.

> The second extends it so as to enable the old LHS to be redefined
> e.g. in terms of the new LHS.  IIRC this may be useful in some other
> transformations that, in the process of introducing VTA, I changed from
> modifying stmts in place to inserting new stmts and removing others.  I
> haven't looked for them yet.

> The third uses this new feature for the case at hand, while avoiding
> computing the reciprocal expression if we know it won't be used.

Updated versions of these follow.  Regstrapped on x86_64-linux-gnu and
i686-linux-gnu.  Ok to install?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-debug-temps-explicit-values-pr42078.patch --]
[-- Type: text/x-diff, Size: 9599 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/42078
	* gimple.h (gimple_replace_lhs_wants_value): New.
	(gimple_replace_lhs): Add value.
	* gimple.c (gimple_replace_lhs): Likewise.  Pass it on.
	* tree-flow.h (insert_debug_temp_for_var_def): Add value.
	* tree-ssa.c (insert_debug_temp_for_var_def): Likewise.  Use it.
	(insert_debug_temps_for_defs): Pass NULL value.
	* tree-ssanames.c (release_ssa_name): Likewise.
	* tree-ssa-math-opts.c (execute_cse_reciprocals): Likewise.
	* gcc/tree-ssa-reassoc.c (repropagate_negates): Likewise.

Index: gcc/gimple.h
===================================================================
--- gcc/gimple.h.orig	2011-06-02 23:26:12.564872257 -0300
+++ gcc/gimple.h	2011-06-03 01:28:09.413126700 -0300
@@ -886,7 +886,7 @@ void gimple_assign_set_rhs_with_ops_1 (g
 				       tree, tree, tree);
 tree gimple_get_lhs (const_gimple);
 void gimple_set_lhs (gimple, tree);
-void gimple_replace_lhs (gimple, tree);
+void gimple_replace_lhs (gimple, tree, tree);
 gimple gimple_copy (gimple);
 void gimple_set_modified (gimple, bool);
 void gimple_cond_get_ops_from_tree (tree, enum tree_code *, tree *, tree *);
@@ -2451,6 +2451,14 @@ gimple_has_lhs (gimple stmt)
 	      && gimple_call_lhs (stmt) != NULL_TREE));
 }
 
+/* Return true if it might be useful to pass a VALUE to
+   gimple_replace_lhs ().  */
+static inline bool
+gimple_replace_lhs_wants_value (void)
+{
+  return MAY_HAVE_DEBUG_STMTS;
+}
+
 
 /* Return the code of the predicate computed by conditional statement GS.  */
 
Index: gcc/gimple.c
===================================================================
--- gcc/gimple.c.orig	2011-06-02 23:26:12.562872251 -0300
+++ gcc/gimple.c	2011-06-03 01:28:09.534126597 -0300
@@ -2133,10 +2133,10 @@ gimple_set_lhs (gimple stmt, tree lhs)
    expression with a different value.
 
    This will update any annotations (say debug bind stmts) referring
-   to the original LHS, so that they use the RHS instead.  This is
-   done even if NLHS and LHS are the same, for it is understood that
-   the RHS will be modified afterwards, and NLHS will not be assigned
-   an equivalent value.
+   to the original LHS, so that they use VALUE or the RHS instead.
+   This is done even if NLHS and LHS are the same, for it is
+   understood that the RHS will be modified afterwards, and NLHS will
+   not be assigned an equivalent value.
 
    Adjusting any non-annotation uses of the LHS, if needed, is a
    responsibility of the caller.
@@ -2148,15 +2148,20 @@ gimple_set_lhs (gimple stmt, tree lhs)
    copying and removing.  */
 
 void
-gimple_replace_lhs (gimple stmt, tree nlhs)
+gimple_replace_lhs (gimple stmt, tree nlhs, tree value)
 {
+  /* If the conditions in which this function uses VALUE change,
+     adjust gimple_replace_lhs_wants_value().  */
+  gcc_assert (gimple_replace_lhs_wants_value ()
+	      == MAY_HAVE_DEBUG_STMTS);
+
   if (MAY_HAVE_DEBUG_STMTS)
     {
       tree lhs = gimple_get_lhs (stmt);
 
       gcc_assert (SSA_NAME_DEF_STMT (lhs) == stmt);
 
-      insert_debug_temp_for_var_def (NULL, lhs);
+      insert_debug_temp_for_var_def (NULL, lhs, value);
     }
 
   gimple_set_lhs (stmt, nlhs);
Index: gcc/tree-flow.h
===================================================================
--- gcc/tree-flow.h.orig	2011-06-02 23:26:53.941986549 -0300
+++ gcc/tree-flow.h	2011-06-03 01:28:55.486087124 -0300
@@ -553,7 +553,7 @@ typedef bool (*walk_use_def_chains_fn) (
 extern void walk_use_def_chains (tree, walk_use_def_chains_fn, void *, bool);
 
 void insert_debug_temps_for_defs (gimple_stmt_iterator *);
-void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree);
+void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree, tree);
 void reset_debug_uses (gimple);
 void release_defs_bitset (bitmap toremove);
 
Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c.orig	2011-06-02 23:26:54.156987140 -0300
+++ gcc/tree-ssa.c	2011-06-03 01:28:09.611126531 -0300
@@ -294,17 +294,25 @@ find_released_ssa_name (tree *tp, int *w
 
 /* Insert a DEBUG BIND stmt before the DEF of VAR if VAR is referenced
    by other DEBUG stmts, and replace uses of the DEF with the
-   newly-created debug temp.  */
+   newly-created debug temp, or with the DEF's value, if the
+   substitution is valid.  The value is taken from VALUE, if given, or
+   from the DEF stmt, taken from GSI, if given, or from the DEF of
+   VAR.  If VALUE and VAR are the same, no DEBUG BIND stmt will be
+   created, and all uses of VAR will be reset.  If a DEBUG BIND stmt
+   has to be created, it will be inserted before or after GSI or the
+   DEF, depending */
 
 void
-insert_debug_temp_for_var_def (gimple_stmt_iterator *gsi, tree var)
+insert_debug_temp_for_var_def (gimple_stmt_iterator *gsi, tree var, tree value)
 {
   imm_use_iterator imm_iter;
   use_operand_p use_p;
   gimple stmt;
   gimple def_stmt = NULL;
   int usecount = 0;
-  tree value = NULL;
+  bool given_value;
+  gimple def_temp = NULL;
+  gimple_stmt_iterator ngsi;
 
   if (!MAY_HAVE_DEBUG_STMTS)
     return;
@@ -344,10 +352,13 @@ insert_debug_temp_for_var_def (gimple_st
   else
     def_stmt = SSA_NAME_DEF_STMT (var);
 
+  given_value = false;
+  if (value)
+    given_value = true;
   /* If we didn't get an insertion point, and the stmt has already
      been removed, we won't be able to insert the debug bind stmt, so
      we'll have to drop debug information.  */
-  if (gimple_code (def_stmt) == GIMPLE_PHI)
+  else if (gimple_code (def_stmt) == GIMPLE_PHI)
     {
       value = degenerate_phi_result (def_stmt);
       if (value && walk_tree (&value, find_released_ssa_name, NULL, NULL))
@@ -402,6 +413,9 @@ insert_debug_temp_for_var_def (gimple_st
 	value = gimple_assign_rhs_to_tree (def_stmt);
     }
 
+  if (value == var)
+    value = NULL;
+
   if (value)
     {
       /* If there's a single use of VAR, and VAR is the entire debug
@@ -419,7 +433,7 @@ insert_debug_temp_for_var_def (gimple_st
 	 at the expense of duplication of expressions.  */
 
       if (CONSTANT_CLASS_P (value)
-	  || gimple_code (def_stmt) == GIMPLE_PHI
+	  || (gimple_code (def_stmt) == GIMPLE_PHI && !given_value)
 	  || (usecount == 1
 	      && (!gimple_assign_single_p (def_stmt)
 		  || is_gimple_min_invariant (value)))
@@ -427,7 +441,6 @@ insert_debug_temp_for_var_def (gimple_st
 	value = unshare_expr (value);
       else
 	{
-	  gimple def_temp;
 	  tree vexpr = make_node (DEBUG_EXPR_DECL);
 
 	  def_temp = gimple_build_debug_bind (vexpr,
@@ -441,14 +454,27 @@ insert_debug_temp_for_var_def (gimple_st
 	  else
 	    DECL_MODE (vexpr) = TYPE_MODE (TREE_TYPE (value));
 
-	  if (gsi)
-	    gsi_insert_before (gsi, def_temp, GSI_SAME_STMT);
-	  else
+	  if (gimple_code (def_stmt) == GIMPLE_PHI)
+	    {
+	      basic_block bb;
+
+	      if (gsi)
+		bb = gsi_bb (*gsi);
+	      else
+		bb = gimple_bb (def_stmt);
+
+	      ngsi = gsi_after_labels (bb);
+	      gsi = &ngsi;
+	    }
+	  else if (!gsi)
 	    {
-	      gimple_stmt_iterator ngsi = gsi_for_stmt (def_stmt);
-	      gsi_insert_before (&ngsi, def_temp, GSI_SAME_STMT);
+	      ngsi = gsi_for_stmt (def_stmt);
+	      gsi = &ngsi;
 	    }
 
+	  if (!given_value)
+	    gsi_insert_before (gsi, def_temp, GSI_SAME_STMT);
+
 	  value = vexpr;
 	}
     }
@@ -477,6 +503,11 @@ insert_debug_temp_for_var_def (gimple_st
 
       update_stmt (stmt);
     }
+
+  /* Insert the definition last, so that we don't modify it in case
+     VALUE references VAR.  Assume the caller intends it to be so.  */
+  if (def_temp && given_value)
+    gsi_insert_after (gsi, def_temp, GSI_SAME_STMT);
 }
 
 
@@ -503,7 +534,7 @@ insert_debug_temps_for_defs (gimple_stmt
       if (TREE_CODE (var) != SSA_NAME)
 	continue;
 
-      insert_debug_temp_for_var_def (gsi, var);
+      insert_debug_temp_for_var_def (gsi, var, NULL);
     }
 }
 
Index: gcc/tree-ssanames.c
===================================================================
--- gcc/tree-ssanames.c.orig	2011-06-02 23:26:54.157987143 -0300
+++ gcc/tree-ssanames.c	2011-06-03 01:28:09.623126522 -0300
@@ -203,7 +203,7 @@ release_ssa_name (tree var)
       use_operand_p imm = &(SSA_NAME_IMM_USE_NODE (var));
 
       if (MAY_HAVE_DEBUG_STMTS)
-	insert_debug_temp_for_var_def (NULL, var);
+	insert_debug_temp_for_var_def (NULL, var, NULL);
 
 #ifdef ENABLE_CHECKING
       verify_imm_links (stderr, var);
Index: gcc/tree-ssa-math-opts.c
===================================================================
--- gcc/tree-ssa-math-opts.c.orig	2011-06-02 23:26:54.149987121 -0300
+++ gcc/tree-ssa-math-opts.c	2011-06-03 01:28:09.658126490 -0300
@@ -603,7 +603,7 @@ execute_cse_reciprocals (void)
 		  if (fail)
 		    continue;
 
-		  gimple_replace_lhs (stmt1, arg1);
+		  gimple_replace_lhs (stmt1, arg1, NULL);
 		  gimple_call_set_fndecl (stmt1, fndecl);
 		  update_stmt (stmt1);
 		  reciprocal_stats.rfuncs_inserted++;
Index: gcc/tree-ssa-reassoc.c
===================================================================
--- gcc/tree-ssa-reassoc.c.orig	2011-06-02 23:26:54.152987130 -0300
+++ gcc/tree-ssa-reassoc.c	2011-06-03 01:28:09.684126469 -0300
@@ -1935,7 +1935,7 @@ repropagate_negates (void)
 	      tree a = gimple_assign_rhs1 (feed);
 	      tree rhs2 = gimple_assign_rhs2 (user);
 	      gimple_stmt_iterator gsi = gsi_for_stmt (feed), gsi2;
-	      gimple_replace_lhs (feed, negate);
+	      gimple_replace_lhs (feed, negate, NULL);
 	      gimple_assign_set_rhs_with_ops (&gsi, PLUS_EXPR, a, rhs2);
 	      update_stmt (gsi_stmt (gsi));
 	      gsi2 = gsi_for_stmt (user);

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: vta-cse-reciprocals-preserve-pr42078.patch --]
[-- Type: text/x-diff, Size: 1384 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/42078
	* tree-ssa-math-opts.c (execute_cse_reciprocals): Compute reciprocal
	value for debug stmts.  

Index: gcc/tree-ssa-math-opts.c
===================================================================
--- gcc/tree-ssa-math-opts.c.orig	2011-06-03 01:29:09.427075107 -0300
+++ gcc/tree-ssa-math-opts.c	2011-06-03 01:30:25.688009048 -0300
@@ -574,6 +574,7 @@ execute_cse_reciprocals (void)
 		  bool md_code, fail;
 		  imm_use_iterator ui;
 		  use_operand_p use_p;
+		  tree value;
 
 		  code = DECL_FUNCTION_CODE (fndecl);
 		  md_code = DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_MD;
@@ -603,13 +604,24 @@ execute_cse_reciprocals (void)
 		  if (fail)
 		    continue;
 
-		  gimple_replace_lhs (stmt1, arg1, NULL);
+		  if (gimple_replace_lhs_wants_value ())
+		    {
+		      tree t = TREE_TYPE (arg1);
+
+		      value = build2 (RDIV_EXPR, t, build_one_cst (t), arg1);
+		    }
+		  else
+		    value = NULL;
+
+		  gimple_replace_lhs (stmt1, arg1, value);
 		  gimple_call_set_fndecl (stmt1, fndecl);
 		  update_stmt (stmt1);
 		  reciprocal_stats.rfuncs_inserted++;
 
 		  FOR_EACH_IMM_USE_STMT (stmt, ui, arg1)
 		    {
+		      if (is_gimple_debug (stmt))
+			continue;
 		      gimple_assign_set_rhs_code (stmt, MULT_EXPR);
 		      fold_stmt_inplace (stmt);
 		      update_stmt (stmt);

[-- Attachment #4: Type: text/plain, Size: 258 bytes --]



-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Alexandre Oliva]

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

On Jun  3, 2011, Alexandre Oliva <aoliva@redhat.com> wrote:

> According to http://gcc.gnu.org/ml/gcc-patches/2009-11/msg01082.html
> on Nov 20, 2009, Alexandre Oliva <aoliva@redhat.com> wrote:

>> On Nov 19, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>>> In fact this exchanging of the LHS (or rather invalidating of the
>>> SSA name value) should be a helper function that knows
>>> the implementation details and avoids going through releasing
>>> and allocating the name.

>> Okie dokie, here's a sequence of patches that implements helper
>> functions for this sort of stuff.

>> The first patch introduces machinery to propagate “dying” DEFs into
>> debug stmts, while replacing them with other SSA_NAMEs.

> This is already in.

>> The second extends it so as to enable the old LHS to be redefined
>> e.g. in terms of the new LHS.  IIRC this may be useful in some other
>> transformations that, in the process of introducing VTA, I changed from
>> modifying stmts in place to inserting new stmts and removing others.  I
>> haven't looked for them yet.

>> The third uses this new feature for the case at hand, while avoiding
>> computing the reciprocal expression if we know it won't be used.

> Updated versions of these follow.  Regstrapped on x86_64-linux-gnu and
> i686-linux-gnu.  Ok to install?

I was asked to submit these again in stage1, so...  Ping?
(updated and re-tested)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-debug-temps-explicit-values-pr42078.patch --]
[-- Type: text/x-diff, Size: 9599 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/42078
	* gimple.h (gimple_replace_lhs_wants_value): New.
	(gimple_replace_lhs): Add value.
	* gimple.c (gimple_replace_lhs): Likewise.  Pass it on.
	* tree-flow.h (insert_debug_temp_for_var_def): Add value.
	* tree-ssa.c (insert_debug_temp_for_var_def): Likewise.  Use it.
	(insert_debug_temps_for_defs): Pass NULL value.
	* tree-ssanames.c (release_ssa_name): Likewise.
	* tree-ssa-math-opts.c (execute_cse_reciprocals): Likewise.
	* gcc/tree-ssa-reassoc.c (repropagate_negates): Likewise.

Index: gcc/gimple.h
===================================================================
--- gcc/gimple.h.orig	2012-04-07 20:24:41.796529955 -0300
+++ gcc/gimple.h	2012-04-08 02:06:44.417457641 -0300
@@ -943,7 +943,7 @@ void gimple_assign_set_rhs_with_ops_1 (g
 				       tree, tree, tree);
 tree gimple_get_lhs (const_gimple);
 void gimple_set_lhs (gimple, tree);
-void gimple_replace_lhs (gimple, tree);
+void gimple_replace_lhs (gimple, tree, tree);
 gimple gimple_copy (gimple);
 void gimple_set_modified (gimple, bool);
 void gimple_cond_get_ops_from_tree (tree, enum tree_code *, tree *, tree *);
@@ -2521,6 +2521,14 @@ gimple_has_lhs (gimple stmt)
 	      && gimple_call_lhs (stmt) != NULL_TREE));
 }
 
+/* Return true if it might be useful to pass a VALUE to
+   gimple_replace_lhs ().  */
+static inline bool
+gimple_replace_lhs_wants_value (void)
+{
+  return MAY_HAVE_DEBUG_STMTS;
+}
+
 
 /* Return the code of the predicate computed by conditional statement GS.  */
 
Index: gcc/gimple.c
===================================================================
--- gcc/gimple.c.orig	2012-04-07 20:24:41.796529955 -0300
+++ gcc/gimple.c	2012-04-08 02:06:44.606455321 -0300
@@ -2239,10 +2239,10 @@ gimple_set_lhs (gimple stmt, tree lhs)
    expression with a different value.
 
    This will update any annotations (say debug bind stmts) referring
-   to the original LHS, so that they use the RHS instead.  This is
-   done even if NLHS and LHS are the same, for it is understood that
-   the RHS will be modified afterwards, and NLHS will not be assigned
-   an equivalent value.
+   to the original LHS, so that they use VALUE or the RHS instead.
+   This is done even if NLHS and LHS are the same, for it is
+   understood that the RHS will be modified afterwards, and NLHS will
+   not be assigned an equivalent value.
 
    Adjusting any non-annotation uses of the LHS, if needed, is a
    responsibility of the caller.
@@ -2254,15 +2254,20 @@ gimple_set_lhs (gimple stmt, tree lhs)
    copying and removing.  */
 
 void
-gimple_replace_lhs (gimple stmt, tree nlhs)
+gimple_replace_lhs (gimple stmt, tree nlhs, tree value)
 {
+  /* If the conditions in which this function uses VALUE change,
+     adjust gimple_replace_lhs_wants_value().  */
+  gcc_assert (gimple_replace_lhs_wants_value ()
+	      == MAY_HAVE_DEBUG_STMTS);
+
   if (MAY_HAVE_DEBUG_STMTS)
     {
       tree lhs = gimple_get_lhs (stmt);
 
       gcc_assert (SSA_NAME_DEF_STMT (lhs) == stmt);
 
-      insert_debug_temp_for_var_def (NULL, lhs);
+      insert_debug_temp_for_var_def (NULL, lhs, value);
     }
 
   gimple_set_lhs (stmt, nlhs);
Index: gcc/tree-flow.h
===================================================================
--- gcc/tree-flow.h.orig	2012-04-07 20:24:45.350481393 -0300
+++ gcc/tree-flow.h	2012-04-08 02:06:44.658454684 -0300
@@ -563,7 +563,7 @@ typedef bool (*walk_use_def_chains_fn) (
 extern void walk_use_def_chains (tree, walk_use_def_chains_fn, void *, bool);
 
 void insert_debug_temps_for_defs (gimple_stmt_iterator *);
-void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree);
+void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree, tree);
 void reset_debug_uses (gimple);
 void release_defs_bitset (bitmap toremove);
 
Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c.orig	2012-02-25 09:45:32.788781511 -0200
+++ gcc/tree-ssa.c	2012-04-08 02:06:44.941451213 -0300
@@ -300,17 +300,25 @@ find_released_ssa_name (tree *tp, int *w
 
 /* Insert a DEBUG BIND stmt before the DEF of VAR if VAR is referenced
    by other DEBUG stmts, and replace uses of the DEF with the
-   newly-created debug temp.  */
+   newly-created debug temp, or with the DEF's value, if the
+   substitution is valid.  The value is taken from VALUE, if given, or
+   from the DEF stmt, taken from GSI, if given, or from the DEF of
+   VAR.  If VALUE and VAR are the same, no DEBUG BIND stmt will be
+   created, and all uses of VAR will be reset.  If a DEBUG BIND stmt
+   has to be created, it will be inserted before or after GSI or the
+   DEF, depending */
 
 void
-insert_debug_temp_for_var_def (gimple_stmt_iterator *gsi, tree var)
+insert_debug_temp_for_var_def (gimple_stmt_iterator *gsi, tree var, tree value)
 {
   imm_use_iterator imm_iter;
   use_operand_p use_p;
   gimple stmt;
   gimple def_stmt = NULL;
   int usecount = 0;
-  tree value = NULL;
+  bool given_value;
+  gimple def_temp = NULL;
+  gimple_stmt_iterator ngsi;
 
   if (!MAY_HAVE_DEBUG_STMTS)
     return;
@@ -350,10 +358,13 @@ insert_debug_temp_for_var_def (gimple_st
   else
     def_stmt = SSA_NAME_DEF_STMT (var);
 
+  given_value = false;
+  if (value)
+    given_value = true;
   /* If we didn't get an insertion point, and the stmt has already
      been removed, we won't be able to insert the debug bind stmt, so
      we'll have to drop debug information.  */
-  if (gimple_code (def_stmt) == GIMPLE_PHI)
+  else if (gimple_code (def_stmt) == GIMPLE_PHI)
     {
       value = degenerate_phi_result (def_stmt);
       if (value && walk_tree (&value, find_released_ssa_name, NULL, NULL))
@@ -408,6 +419,9 @@ insert_debug_temp_for_var_def (gimple_st
 	value = gimple_assign_rhs_to_tree (def_stmt);
     }
 
+  if (value == var)
+    value = NULL;
+
   if (value)
     {
       /* If there's a single use of VAR, and VAR is the entire debug
@@ -425,7 +439,7 @@ insert_debug_temp_for_var_def (gimple_st
 	 at the expense of duplication of expressions.  */
 
       if (CONSTANT_CLASS_P (value)
-	  || gimple_code (def_stmt) == GIMPLE_PHI
+	  || (gimple_code (def_stmt) == GIMPLE_PHI && !given_value)
 	  || (usecount == 1
 	      && (!gimple_assign_single_p (def_stmt)
 		  || is_gimple_min_invariant (value)))
@@ -433,7 +447,6 @@ insert_debug_temp_for_var_def (gimple_st
 	value = unshare_expr (value);
       else
 	{
-	  gimple def_temp;
 	  tree vexpr = make_node (DEBUG_EXPR_DECL);
 
 	  def_temp = gimple_build_debug_bind (vexpr,
@@ -447,14 +460,27 @@ insert_debug_temp_for_var_def (gimple_st
 	  else
 	    DECL_MODE (vexpr) = TYPE_MODE (TREE_TYPE (value));
 
-	  if (gsi)
-	    gsi_insert_before (gsi, def_temp, GSI_SAME_STMT);
-	  else
+	  if (gimple_code (def_stmt) == GIMPLE_PHI)
+	    {
+	      basic_block bb;
+
+	      if (gsi)
+		bb = gsi_bb (*gsi);
+	      else
+		bb = gimple_bb (def_stmt);
+
+	      ngsi = gsi_after_labels (bb);
+	      gsi = &ngsi;
+	    }
+	  else if (!gsi)
 	    {
-	      gimple_stmt_iterator ngsi = gsi_for_stmt (def_stmt);
-	      gsi_insert_before (&ngsi, def_temp, GSI_SAME_STMT);
+	      ngsi = gsi_for_stmt (def_stmt);
+	      gsi = &ngsi;
 	    }
 
+	  if (!given_value)
+	    gsi_insert_before (gsi, def_temp, GSI_SAME_STMT);
+
 	  value = vexpr;
 	}
     }
@@ -486,6 +512,11 @@ insert_debug_temp_for_var_def (gimple_st
 
       update_stmt (stmt);
     }
+
+  /* Insert the definition last, so that we don't modify it in case
+     VALUE references VAR.  Assume the caller intends it to be so.  */
+  if (def_temp && given_value)
+    gsi_insert_after (gsi, def_temp, GSI_SAME_STMT);
 }
 
 
@@ -512,7 +543,7 @@ insert_debug_temps_for_defs (gimple_stmt
       if (TREE_CODE (var) != SSA_NAME)
 	continue;
 
-      insert_debug_temp_for_var_def (gsi, var);
+      insert_debug_temp_for_var_def (gsi, var, NULL);
     }
 }
 
Index: gcc/tree-ssanames.c
===================================================================
--- gcc/tree-ssanames.c.orig	2012-04-07 20:24:45.383480943 -0300
+++ gcc/tree-ssanames.c	2012-04-08 02:06:45.215447852 -0300
@@ -203,7 +203,7 @@ release_ssa_name (tree var)
       use_operand_p imm = &(SSA_NAME_IMM_USE_NODE (var));
 
       if (MAY_HAVE_DEBUG_STMTS)
-	insert_debug_temp_for_var_def (NULL, var);
+	insert_debug_temp_for_var_def (NULL, var, NULL);
 
 #ifdef ENABLE_CHECKING
       verify_imm_links (stderr, var);
Index: gcc/tree-ssa-math-opts.c
===================================================================
--- gcc/tree-ssa-math-opts.c.orig	2012-04-07 20:24:45.354481340 -0300
+++ gcc/tree-ssa-math-opts.c	2012-04-08 02:06:45.383445792 -0300
@@ -604,7 +604,7 @@ execute_cse_reciprocals (void)
 		  if (fail)
 		    continue;
 
-		  gimple_replace_lhs (stmt1, arg1);
+		  gimple_replace_lhs (stmt1, arg1, NULL);
 		  gimple_call_set_fndecl (stmt1, fndecl);
 		  update_stmt (stmt1);
 		  reciprocal_stats.rfuncs_inserted++;
Index: gcc/tree-ssa-reassoc.c
===================================================================
--- gcc/tree-ssa-reassoc.c.orig	2011-10-04 02:32:44.000000000 -0300
+++ gcc/tree-ssa-reassoc.c	2012-04-08 02:06:45.923439168 -0300
@@ -2719,7 +2719,7 @@ repropagate_negates (void)
 	      tree a = gimple_assign_rhs1 (feed);
 	      tree rhs2 = gimple_assign_rhs2 (user);
 	      gimple_stmt_iterator gsi = gsi_for_stmt (feed), gsi2;
-	      gimple_replace_lhs (feed, negate);
+	      gimple_replace_lhs (feed, negate, NULL);
 	      gimple_assign_set_rhs_with_ops (&gsi, PLUS_EXPR, a, rhs2);
 	      update_stmt (gsi_stmt (gsi));
 	      gsi2 = gsi_for_stmt (user);

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: vta-cse-reciprocals-preserve-pr42078.patch --]
[-- Type: text/x-diff, Size: 1512 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR tree-optimization/42078
	* tree-ssa-math-opts.c (execute_cse_reciprocals): Compute reciprocal
	value for debug stmts.  

Index: gcc/tree-ssa-math-opts.c
===================================================================
--- gcc/tree-ssa-math-opts.c.orig	2012-04-08 02:06:45.383445792 -0300
+++ gcc/tree-ssa-math-opts.c	2012-04-08 02:09:53.000000000 -0300
@@ -575,6 +575,7 @@ execute_cse_reciprocals (void)
 		  bool md_code, fail;
 		  imm_use_iterator ui;
 		  use_operand_p use_p;
+		  tree value;
 
 		  code = DECL_FUNCTION_CODE (fndecl);
 		  md_code = DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_MD;
@@ -604,14 +605,26 @@ execute_cse_reciprocals (void)
 		  if (fail)
 		    continue;
 
-		  gimple_replace_lhs (stmt1, arg1, NULL);
+		  if (gimple_replace_lhs_wants_value ())
+		    {
+		      tree t = TREE_TYPE (arg1);
+
+		      value = build2 (RDIV_EXPR, t, build_one_cst (t), arg1);
+		    }
+		  else
+		    value = NULL;
+
+		  gimple_replace_lhs (stmt1, arg1, value);
 		  gimple_call_set_fndecl (stmt1, fndecl);
 		  update_stmt (stmt1);
 		  reciprocal_stats.rfuncs_inserted++;
 
 		  FOR_EACH_IMM_USE_STMT (stmt, ui, arg1)
 		    {
-		      gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
+		      gimple_stmt_iterator gsi;
+		      if (is_gimple_debug (stmt))
+			continue;
+		      gsi = gsi_for_stmt (stmt);
 		      gimple_assign_set_rhs_code (stmt, MULT_EXPR);
 		      fold_stmt_inplace (&gsi);
 		      update_stmt (stmt);

[-- Attachment #4: Type: text/plain, Size: 258 bytes --]



-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Richard Guenther]

On Mon, Apr 9, 2012 at 8:37 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Jun  3, 2011, Alexandre Oliva <aoliva@redhat.com> wrote:
>
>> According to http://gcc.gnu.org/ml/gcc-patches/2009-11/msg01082.html
>> on Nov 20, 2009, Alexandre Oliva <aoliva@redhat.com> wrote:
>
>>> On Nov 19, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:
>>>> In fact this exchanging of the LHS (or rather invalidating of the
>>>> SSA name value) should be a helper function that knows
>>>> the implementation details and avoids going through releasing
>>>> and allocating the name.
>
>>> Okie dokie, here's a sequence of patches that implements helper
>>> functions for this sort of stuff.
>
>>> The first patch introduces machinery to propagate “dying” DEFs into
>>> debug stmts, while replacing them with other SSA_NAMEs.
>
>> This is already in.
>
>>> The second extends it so as to enable the old LHS to be redefined
>>> e.g. in terms of the new LHS.  IIRC this may be useful in some other
>>> transformations that, in the process of introducing VTA, I changed from
>>> modifying stmts in place to inserting new stmts and removing others.  I
>>> haven't looked for them yet.
>
>>> The third uses this new feature for the case at hand, while avoiding
>>> computing the reciprocal expression if we know it won't be used.
>
>> Updated versions of these follow.  Regstrapped on x86_64-linux-gnu and
>> i686-linux-gnu.  Ok to install?
>
> I was asked to submit these again in stage1, so...  Ping?
> (updated and re-tested)


+  /* If the conditions in which this function uses VALUE change,
+     adjust gimple_replace_lhs_wants_value().  */
+  gcc_assert (gimple_replace_lhs_wants_value ()
+             == MAY_HAVE_DEBUG_STMTS);
+

that looks ... odd.  But I see you want to conditionally compute value.

+static inline bool
+gimple_replace_lhs_wants_value (void)
+{
+  return MAY_HAVE_DEBUG_STMTS;
+}

but should this not depend on the old stmt / lhs?  For example do we really
want to do this for artificial variables?  I suppose not.

Thanks,
Richard.

>
>
> --
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist      Red Hat Brazil Compiler Engineer
>

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Alexandre Oliva]

Sorry I dropped the ball on this one.  Context is here:
http://gcc.gnu.org/ml/gcc-patches/2012-04/msg00416.html

On Apr 12, 2012, Richard Guenther <richard.guenther@gmail.com> wrote:

> +  /* If the conditions in which this function uses VALUE change,
> +     adjust gimple_replace_lhs_wants_value().  */
> +  gcc_assert (gimple_replace_lhs_wants_value ()
> +             == MAY_HAVE_DEBUG_STMTS);
> +

> that looks ... odd.  But I see you want to conditionally compute value.

> +static inline bool
> +gimple_replace_lhs_wants_value (void)
> +{
> +  return MAY_HAVE_DEBUG_STMTS;
> +}

> but should this not depend on the old stmt / lhs?  For example do we really
> want to do this for artificial variables?  I suppose not.

I think we do.  We want to preserve the value of the expression when it
is referenced in other debug expressions.  For these other uses, it
doesn't matter whether this value happened to be computed and stored in
a temporary, an artificial variable or a user-defined variable.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: [PATCH] Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

[Alexandre Oliva]

On Apr 12, 2012, Richard Guenther <richard.guenther@gmail.com> wrote:

> +  /* If the conditions in which this function uses VALUE change,
> +     adjust gimple_replace_lhs_wants_value().  */
> +  gcc_assert (gimple_replace_lhs_wants_value ()
> +             == MAY_HAVE_DEBUG_STMTS);
> +
     if (MAY_HAVE_DEBUG_STMTS)
       {

> that looks ... odd.

Indeed, it does.  Does this look any better?

  bool save_value = MAY_HAVE_DEBUG_STMTS;
  /* If the condition above, in which this function uses VALUE change,
     adjust gimple_replace_lhs_wants_value() to match.  The assert
     below helps enforce this.  */
  gcc_checking_assert (gimple_replace_lhs_wants_value () == save_value);

  if (save_value)
    {


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer