Re: [PATCH] Fix reassociation with -g (PR tree-optimization/60844)

[Richard Biener <rguenther at suse dot de>]

On Tue, 15 Apr 2014, Jakub Jelinek wrote:

> Hi!
> 
> The (admittedly ugly) reassoc stmt positioning stuff requires that
> we maintain uids in ascending order within each bb (equal uid for several
> adjacent stmts is ok), including debug stmts.
> We assign those initially, and for stmts we add we make sure to copy the
> uid from some adjacent insn.
> 
> But, as the following testcase shows, we don't take into account that
> gsi_remove can add debug stmts, and those don't have uid set.
> 
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux,
> ok for trunk/4.9.1?

Ok.

Thanks,
Richard.

> 2014-04-15  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/60844
> 	* tree-ssa-reassoc.c (reassoc_remove_stmt): New function.
> 	(propagate_op_to_single_use, remove_visited_stmt_chain,
> 	linearize_expr, repropagate_negates, reassociate_bb): Use it
> 	instead of gsi_remove.
> 
> 	* gcc.dg/pr60844.c: New test.
> 
> --- gcc/tree-ssa-reassoc.c.jj	2014-03-13 10:38:09.000000000 +0100
> +++ gcc/tree-ssa-reassoc.c	2014-04-15 13:59:14.511383249 +0200
> @@ -221,6 +221,35 @@ static struct pointer_map_t *operand_ran
>  static long get_rank (tree);
>  static bool reassoc_stmt_dominates_stmt_p (gimple, gimple);
>  
> +/* Wrapper around gsi_remove, which adjusts gimple_uid of debug stmts
> +   possibly added by gsi_remove.  */
> +
> +bool
> +reassoc_remove_stmt (gimple_stmt_iterator *gsi)
> +{
> +  gimple stmt = gsi_stmt (*gsi);
> +
> +  if (!MAY_HAVE_DEBUG_STMTS || gimple_code (stmt) == GIMPLE_PHI)
> +    return gsi_remove (gsi, true);
> +
> +  gimple_stmt_iterator prev = *gsi;
> +  gsi_prev (&prev);
> +  unsigned uid = gimple_uid (stmt);
> +  basic_block bb = gimple_bb (stmt);
> +  bool ret = gsi_remove (gsi, true);
> +  if (!gsi_end_p (prev))
> +    gsi_next (&prev);
> +  else
> +    prev = gsi_start_bb (bb);
> +  gimple end_stmt = gsi_stmt (*gsi);
> +  while ((stmt = gsi_stmt (prev)) != end_stmt)
> +    {
> +      gcc_assert (stmt && is_gimple_debug (stmt) && gimple_uid (stmt) == 0);
> +      gimple_set_uid (stmt, uid);
> +      gsi_next (&prev);
> +    }
> +  return ret;
> +}
>  
>  /* Bias amount for loop-carried phis.  We want this to be larger than
>     the depth of any reassociation tree we can see, but not larger than
> @@ -1123,7 +1152,7 @@ propagate_op_to_single_use (tree op, gim
>      update_stmt (use_stmt);
>    gsi = gsi_for_stmt (stmt);
>    unlink_stmt_vdef (stmt);
> -  gsi_remove (&gsi, true);
> +  reassoc_remove_stmt (&gsi);
>    release_defs (stmt);
>  }
>  
> @@ -3072,7 +3101,7 @@ remove_visited_stmt_chain (tree var)
>  	{
>  	  var = gimple_assign_rhs1 (stmt);
>  	  gsi = gsi_for_stmt (stmt);
> -	  gsi_remove (&gsi, true);
> +	  reassoc_remove_stmt (&gsi);
>  	  release_defs (stmt);
>  	}
>        else
> @@ -3494,7 +3523,7 @@ linearize_expr (gimple stmt)
>    update_stmt (stmt);
>  
>    gsi = gsi_for_stmt (oldbinrhs);
> -  gsi_remove (&gsi, true);
> +  reassoc_remove_stmt (&gsi);
>    release_defs (oldbinrhs);
>  
>    gimple_set_visited (stmt, true);
> @@ -3896,7 +3925,7 @@ repropagate_negates (void)
>  	      gimple_assign_set_rhs_with_ops (&gsi2, NEGATE_EXPR, x, NULL);
>  	      user = gsi_stmt (gsi2);
>  	      update_stmt (user);
> -	      gsi_remove (&gsi, true);
> +	      reassoc_remove_stmt (&gsi);
>  	      release_defs (feed);
>  	      plus_negates.safe_push (gimple_assign_lhs (user));
>  	    }
> @@ -4413,7 +4442,7 @@ reassociate_bb (basic_block bb)
>  		 reassociations.  */
>  	      if (has_zero_uses (gimple_get_lhs (stmt)))
>  		{
> -		  gsi_remove (&gsi, true);
> +		  reassoc_remove_stmt (&gsi);
>  		  release_defs (stmt);
>  		  /* We might end up removing the last stmt above which
>  		     places the iterator to the end of the sequence.
> --- gcc/testsuite/gcc.dg/pr60844.c.jj	2014-04-15 14:01:27.561689401 +0200
> +++ gcc/testsuite/gcc.dg/pr60844.c	2014-04-15 14:01:10.000000000 +0200
> @@ -0,0 +1,16 @@
> +/* PR tree-optimization/60844 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -g" } */
> +/* { dg-additional-options "-mtune=atom" { target { i?86-*-* x86_64-*-* } } } */
> +
> +void
> +foo (int *x, int y, int z)
> +{
> +  int b, c = x[0], d = x[1];
> +  for (b = 0; b < 1; b++)
> +    {
> +      int e = (y ? 1 : 0) | (d ? 2 : 0) | (z ? 1 : 0);
> +      e |= (c ? 2 : 0) | ((1 >> b) ? 1 : 0);
> +      x[2 + b] = e;
> +    }
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer