Patchwork Fix ICE in expand_cse_reciprocals (PR tree-optimization/42078)

login
register
mail settings
Submitter Alexandre Oliva
Date April 9, 2012, 6:37 a.m.
Message ID <ork41pe7hk.fsf@livre.localdomain>
Download mbox | patch
Permalink /patch/151408/
State New
Headers show

Comments

Alexandre Oliva - April 9, 2012, 6:37 a.m.
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)
Richard Guenther - April 12, 2012, 2:41 p.m.
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
>
Alexandre Oliva - June 13, 2012, 9:38 p.m.
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 - June 14, 2012, 6:20 a.m.
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)
    {

Patch

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);