Re: [PR66726] Factor conversion out of COND_EXPR

[Jeff Law <law at redhat dot com>]

On 07/07/2015 06:50 AM, Kugan wrote:
> Thanks for the review. I have addressed your comments above in the
> attached patch.
>
> I have one question with respect to unary operation. For generic unary
> operation with INTEGER_CST, do we skip this or do we have to perform the
> inverse operation so that the conversion after PHI will restore it? Is
> there any easy way to do this safely?
I think we'd have to invert the operation -- some of which are trivial, 
such as BIT_NOT_EXPR.

NEGATE_EXPR is trivial once you filter out the cases where inversion 
will create signed overflow (ie INT_MIN and like when arg1 is an 
INTEGER_CST).

Similarly ABS_EXPR is trivial once you filter out cases where arg1 is a 
negative INTEGER_CST.

If you want to try and handle those cases, I'm certainly comfortable 
with that as a follow-up.  Obviously we'll want to testcases for them, 
including the cases where we don't want to make the transformation for 
NEGATE_EXPR and ABS_EXPR.

There may be other special cases we need to handle for other unary 
operations.  I haven't walked through the full list.

> Bootstrapped and regression tested the attached patch on
> x86-64-none-linux-gnu with no new regressions.
>
> Thanks,
> Kugan
>
>
> p.txt
>
>
> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> index 92b4ab0..1d6de9b 100644
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c
> @@ -73,6 +73,7 @@ along with GCC; see the file COPYING3.  If not see
>   static unsigned int tree_ssa_phiopt_worker (bool, bool);
>   static bool conditional_replacement (basic_block, basic_block,
>   				     edge, edge, gphi *, tree, tree);
> +static bool factor_out_conditional_conversion (edge, edge, gphi *, tree, tree);
>   static int value_replacement (basic_block, basic_block,
>   			      edge, edge, gimple, tree, tree);
>   static bool minmax_replacement (basic_block, basic_block,
> @@ -335,6 +336,17 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads)
>   	     node.  */
>   	  gcc_assert (arg0 != NULL && arg1 != NULL);
>
> +	  if (factor_out_conditional_conversion (e1, e2, phi, arg0, arg1))
> +	    {
> +	      /* Update arg0 and arg1.  */
> +	      phis = phi_nodes (bb2);
> +	      phi = single_non_singleton_phi_for_edges (phis, e1, e2);
> +	      gcc_assert (phi);
> +	      arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
> +	      arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
> +	      gcc_assert (arg0 != NULL && arg1 != NULL);
> +	    }
> +
So small comment before this block of code indicating why we're 
recomputing these values.  Something like this perhaps:

/* factor_out_conditional_conversion may create a new PHI in BB2 and
   eliminate an existing PHI in BB2.  Recompute values that may be
   affected by that change.  */


Or something along those lines.


>   	  /* Do the replacement of conditional if it can be done.  */
>   	  if (conditional_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
>   	    cfgchanged = true;
> @@ -410,6 +422,133 @@ replace_phi_edge_with_variable (basic_block cond_block,
>   	      bb->index);
>   }
>
> +/* 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.  */
> +
> +static bool
> +factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
> +				   tree arg0, tree arg1)
> +{
> +  gimple arg0_def_stmt = NULL, arg1_def_stmt = NULL, new_stmt;
> +  tree new_arg0 = NULL_TREE, new_arg1 = NULL_TREE;
> +  tree temp, result;
> +  gphi *newphi;
> +  gimple_stmt_iterator gsi, gsi_for_def;
> +  source_location locus = gimple_location (phi);
> +  enum tree_code convert_code;
> +
> +  /* Handle only PHI statements with two arguments.  TODO: If all
> +     other arguments to PHI are INTEGER_CST, we can handle more
> +     than two arguments too.  */
> +  if (gimple_phi_num_args (phi) != 2)
> +    return false;
Similarly we can handle more than one SSA_NAME if their defining 
statement all have the same unary operation.  Might as well add that to 
the comment too.  While handling > 2 arguments is certainly possible, I 
really wonder how often those cases occur in practice.

> +
> +  /* If arg0 is an SSA_NAME and the stmt which defines arg0 is
> +     a conversion.  */
> +  arg0_def_stmt = SSA_NAME_DEF_STMT (arg0);
> +  if (!is_gimple_assign (arg0_def_stmt)
> +      || (!gimple_assign_cast_p (arg0_def_stmt)
> +	  && (get_gimple_rhs_class (gimple_assign_rhs_code (arg0_def_stmt))
> +	      != GIMPLE_UNARY_RHS)))
So what happens if arg0_def_stmt is a GIMPLE_UNARY_RHS other than a 
NOP_EXPR or CONVERT_EXPR?    I'd probably punt anything that is not a 
gimple_assign_cast_p for the first iteration and follow-up with handling 
of the other unary operations as a follow-up.



> +    return false;
> +
> +  /* Use the LHS as new_arg0.  */
s/LHS/RHS/

> +  convert_code = gimple_assign_rhs_code (arg0_def_stmt);
> +  new_arg0 = gimple_assign_rhs1 (arg0_def_stmt);
> +  if (convert_code == VIEW_CONVERT_EXPR)
> +    new_arg0 = TREE_OPERAND (new_arg0, 0);
Is this really right for VIEW_CONVERT_EXPR?  Also, do we do the right 
thing for FIX_TRUNC_EXPR?


> +
> +  /* If arg1 is an SSA_NAME and the stmt which defines arg0 is
> +     a conversion.  */
s/arg0/arg1/

It's also the case that if arg1 is an SSA_NAME, then it must do the same 
operation as we found in arg0's defining statement.  I see that's 
properly tested in the code, so just a minor comment updated needed.

> +
> +  /* Create a new PHI stmt.  */
> +  result = PHI_RESULT (phi);
> +  temp = make_ssa_name (TREE_TYPE (new_arg0), NULL);
> +  newphi = create_phi_node (temp, gimple_bb (phi));
> +
> +  if (dump_file && (dump_flags & TDF_DETAILS))
> +    {
> +      fprintf (dump_file, "PHI ");
> +      print_generic_expr (dump_file, gimple_phi_result (phi), 0);
> +      fprintf (dump_file,
> +	       " changed to factor conversion out from COND_EXPR.\n");
> +      fprintf (dump_file, "New stmt with CAST that defines ");
> +      print_generic_expr (dump_file, result, 0);
> +      fprintf (dump_file, ".\n");
> +    }
> +
> +  /* Remove the old cast(s) that has single use.  */
> +  if (arg0_def_stmt && has_single_use (arg0))
> +    {
> +      gsi_for_def = gsi_for_stmt (arg0_def_stmt);
> +      gsi_remove (&gsi_for_def, true);
> +    }
> +
> +  if (arg1_def_stmt && has_single_use (arg1))
> +    {
> +      gsi_for_def = gsi_for_stmt (arg1_def_stmt);
> +      gsi_remove (&gsi_for_def, true);
> +    }
So I would move the have_single_use tests up higher and reject the 
transformation if arg0/arg1 have > 1 use.  The reason is if arg0/arg1 
have > 1 use, then this transformation actually increases the number of 
expressions evaluated at runtime.

It'd probably be good to include a test when arg0 or arg1 are both 
SSA_NAMEs and new_arg0 and new_arg1 have > 1 use to verify the 
transformation does not apply in that case.

Very close.     I suspect one more iteration.

jeff