This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Tweak factor_out_conditional_conversion (PR tree-optimization/71016, take 2)


On Thu, 5 Jan 2017, Jakub Jelinek wrote:

> Hi!
> 
> On Thu, Jan 05, 2017 at 09:09:06AM +0100, Richard Biener wrote:
> > I think that would be premature.  Consider a modified 
> > gcc.dg/tree-ssa/pr66726.c:
> > 
> > extern unsigned short mode_size[];
> > 
> > int
> > oof (int mode)
> > {
> >   int tem;
> >   if (64 < mode_size[mode])
> >     tem = 64;
> >   else
> >     tem = mode_size[mode];
> >   return tem;
> > }
> 
> You're right, that testcase still needs factor_out_*.
> 
> > That we now fold the GENERIC cond-expr during gimplification
> > is of course good but doesn't make the phiopt code useless.
> > As far as I remember I objected adding handling of conversions
> > in the minmax replacement code and instead suggested to add this
> > "enablement" phiopt instead.  So what I suggest is to tame that down
> > to the cases where it actually enables something (empty BB afterwards,
> > a PHI with a use that's also used on the controlling conditional).
> 
> So like this (if it passes bootstrap/regtest)?

Yes, this looks good to me.

Thanks,
Richard.

> 2017-01-05  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/71016
> 	* tree-ssa-phiopt.c (tree_ssa_phiopt_worker): Pass cond_stmt to
> 	factor_out_conditional_conversion.  Formatting fix.
> 	(factor_out_conditional_conversion): Add cond_stmt argument.
> 	If arg1 is INTEGER_CST, punt if new_arg0 is not any operand of
> 	cond_stmt and if arg0_def_stmt is not the only stmt in its bb.
> 
> 	* gcc.target/i386/pr71016.c: New test.
> 	* gcc.target/aarch64/pr71016.c: New test.
> 	* gcc.dg/tree-ssa/pr66726-3.c: New test.
> 
> --- gcc/tree-ssa-phiopt.c.jj	2017-01-03 08:12:27.000000000 +0100
> +++ gcc/tree-ssa-phiopt.c	2017-01-05 12:12:09.571017488 +0100
> @@ -49,7 +49,8 @@ along with GCC; see the file COPYING3.
>  static unsigned int tree_ssa_phiopt_worker (bool, bool);
>  static bool conditional_replacement (basic_block, basic_block,
>  				     edge, edge, gphi *, tree, tree);
> -static gphi *factor_out_conditional_conversion (edge, edge, gphi *, tree, tree);
> +static gphi *factor_out_conditional_conversion (edge, edge, gphi *, tree, tree,
> +						gimple *);
>  static int value_replacement (basic_block, basic_block,
>  			      edge, edge, gimple *, tree, tree);
>  static bool minmax_replacement (basic_block, basic_block,
> @@ -233,7 +234,7 @@ tree_ssa_phiopt_worker (bool do_store_el
>  	  continue;
>  	}
>        else if (do_hoist_loads
> -		 && EDGE_SUCC (bb1, 0)->dest == EDGE_SUCC (bb2, 0)->dest)
> +	       && EDGE_SUCC (bb1, 0)->dest == EDGE_SUCC (bb2, 0)->dest)
>  	{
>  	  basic_block bb3 = EDGE_SUCC (bb1, 0)->dest;
>  
> @@ -313,7 +314,8 @@ tree_ssa_phiopt_worker (bool do_store_el
>  	  gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
>  
>  	  gphi *newphi = factor_out_conditional_conversion (e1, e2, phi,
> -							    arg0, arg1);
> +							    arg0, arg1,
> +							    cond_stmt);
>  	  if (newphi != NULL)
>  	    {
>  	      phi = newphi;
> @@ -402,11 +404,12 @@ replace_phi_edge_with_variable (basic_bl
>  
>  /* PR66726: Factor conversion out of COND_EXPR.  If the arguments of the PHI
>     stmt are CONVERT_STMT, factor out the conversion and perform the conversion
> -   to the result of PHI stmt.  Return the newly-created PHI, if any.  */
> +   to the result of PHI stmt.  COND_STMT is the controlling predicate.
> +   Return the newly-created PHI, if any.  */
>  
>  static gphi *
>  factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
> -				   tree arg0, tree arg1)
> +				   tree arg0, tree arg1, gimple *cond_stmt)
>  {
>    gimple *arg0_def_stmt = NULL, *arg1_def_stmt = NULL, *new_stmt;
>    tree new_arg0 = NULL_TREE, new_arg1 = NULL_TREE;
> @@ -472,7 +475,31 @@ factor_out_conditional_conversion (edge
>  	  && int_fits_type_p (arg1, TREE_TYPE (new_arg0)))
>  	{
>  	  if (gimple_assign_cast_p (arg0_def_stmt))
> -	    new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);
> +	    {
> +	      /* For the INTEGER_CST case, we are just moving the
> +		 conversion from one place to another, which can often
> +		 hurt as the conversion moves further away from the
> +		 statement that computes the value.  So, perform this
> +		 only if new_arg0 is an operand of COND_STMT, or
> +		 if arg0_def_stmt is the only non-debug stmt in
> +		 its basic block, because then it is possible this
> +		 could enable further optimizations (minmax replacement
> +		 etc.).  See PR71016.  */
> +	      if (new_arg0 != gimple_cond_lhs (cond_stmt)
> +		  && new_arg0 != gimple_cond_rhs (cond_stmt)
> +		  && gimple_bb (arg0_def_stmt) == e0->src)
> +		{
> +		  gsi = gsi_for_stmt (arg0_def_stmt);
> +		  gsi_prev_nondebug (&gsi);
> +		  if (!gsi_end_p (gsi))
> +		    return NULL;
> +		  gsi = gsi_for_stmt (arg0_def_stmt);
> +		  gsi_next_nondebug (&gsi);
> +		  if (!gsi_end_p (gsi))
> +		    return NULL;
> +		}
> +	      new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);
> +	    }
>  	  else
>  	    return NULL;
>  	}
> --- gcc/testsuite/gcc.target/i386/pr71016.c.jj	2017-01-05 12:06:33.808325995 +0100
> +++ gcc/testsuite/gcc.target/i386/pr71016.c	2017-01-05 12:07:54.242293867 +0100
> @@ -0,0 +1,10 @@
> +/* PR tree-optimization/71016 */
> +/* { dg-do compile { target lp64 } } */
> +/* { dg-options "-O2 -mlzcnt" } */
> +/* { dg-final { scan-assembler-not "cltq" } } */
> +
> +long int
> +foo (long int i)
> +{
> +  return i == 0 ? 17 : __builtin_clzl (i);
> +}
> --- gcc/testsuite/gcc.target/aarch64/pr71016.c.jj	2017-01-05 12:09:12.075295113 +0100
> +++ gcc/testsuite/gcc.target/aarch64/pr71016.c	2017-01-05 12:09:28.479084620 +0100
> @@ -0,0 +1,10 @@
> +/* PR tree-optimization/71016 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +/* { dg-final { scan-assembler-not "sxtw" } } */
> +
> +long int
> +foo (long int i)
> +{
> +  return i == 0 ? 17 : __builtin_clzl (i);
> +}
> --- gcc/testsuite/gcc.dg/tree-ssa/pr66726-3.c.jj	2017-01-05 12:10:07.746580739 +0100
> +++ gcc/testsuite/gcc.dg/tree-ssa/pr66726-3.c	2017-01-05 11:41:59.000000000 +0100
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +/* { dg-final { scan-tree-dump-times "MIN_EXPR" 1 "optimized" } } */
> +
> +extern unsigned short mode_size[];
> +
> +int
> +oof (int mode)
> +{
> +  int tem;
> +  if (64 < mode_size[mode])
> +    tem = 64;
> +  else
> +    tem = mode_size[mode];
> +  return tem;
> +}
> 
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]