Re: [PR66726] Factor conversion out of COND_EXPR

[Bernhard Reutner-Fischer <rep dot dot dot nop at gmail dot com>]

On Sat, Jul 04, 2015 at 12:58:58PM +1000, Kugan wrote:
> Please find a patch that attempt to FIX PR66726 by factoring conversion
> out of COND_EXPR as explained in the PR.
> 
> Bootstrapped and regression tested on x86-64-none-linux-gnu with no new
> regressions. Is this OK for trunk?
> 
> Thanks,
> Kugan
> 
> 
> gcc/testsuite/ChangeLog:
> 
> 2015-07-03  Kugan Vivekanandarajah  <kuganv@linaro.org>
> 	    Jeff Law  <law@redhat.com>
> 
> 	PR middle-end/66726
> 	* gcc.dg/tree-ssa/pr66726.c: New test.

I'd have scanned the details dump for "factor CONVERT_EXPR out" 1
to make sure that it's this part that takes care of it.

> 
> gcc/ChangeLog:
> 
> 2015-07-03  Kugan Vivekanandarajah  <kuganv@linaro.org>
> 
> 	PR middle-end/66726
> 	* tree-ssa-phiopt.c (factor_out_conditional_conversion): New function.
> 	(tree_ssa_phiopt_worker): Call factor_out_conditional_conversion.

> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> index d2a5cee..e8af086 100644
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c

> @@ -410,6 +413,108 @@ replace_phi_edge_with_variable (basic_block cond_block,
>  	      bb->index);
>  }
>  
> +/* PR66726: Factor conversion out of COND_EXPR.  If the argument of the PHI

s/the argument/the arguments/

> +   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 def0 = NULL, def1 = NULL, new_stmt;
> +  tree new_arg0 = NULL_TREE, new_arg1 = NULL_TREE;
> +  tree temp, result;
> +  gimple_stmt_iterator gsi;
> +
> +  /* One of the argument has to be SSA_NAME and other argument can

s/the argument/the arguments/

> +     be an SSA_NAME of INTEGER_CST.  */
> +  if ((TREE_CODE (arg0) != SSA_NAME
> +       && TREE_CODE (arg0) != INTEGER_CST)
> +      || (TREE_CODE (arg1) != SSA_NAME
> +	  && TREE_CODE (arg1) != INTEGER_CST)
> +      || (TREE_CODE (arg0) == INTEGER_CST
> +	  && TREE_CODE (arg1) == INTEGER_CST))

inconsistent space for the && lines above; The first should have a
leading tab.

> +    return false;
> +
> +  /* 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;
> +
> +  /* If arg0 is an SSA_NAME and the stmt which defines arg0 is
> +     ai CONVERT_STMT, use the LHS as new_arg0.  */

s/ai/a/

> +  if (TREE_CODE (arg0) == SSA_NAME)
> +    {
> +      def0 = SSA_NAME_DEF_STMT (arg0);
> +      if (!is_gimple_assign (def0)
> +	  || !CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (def0)))
> +	return false;
> +      new_arg0 = gimple_assign_rhs1 (def0);
> +    }
> +
> +  /* If arg1 is an SSA_NAME and the stmt which defines arg0 is
> +     ai CONVERT_STMT, use the LHS as new_arg1.  */

s/ai/a/

> +  if (TREE_CODE (arg1) == SSA_NAME)
> +    {
> +      def1 = SSA_NAME_DEF_STMT (arg1);
> +      if (!is_gimple_assign (def1)
> +	  || !CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (def1)))
> +	return false;
> +      new_arg1 = gimple_assign_rhs1 (def1);
> +    }
> +
> +  /* If arg0 is an INTEGER_CST, fold it to new type.  */
> +  if (TREE_CODE (arg0) != SSA_NAME)
> +    {
> +      if (!POINTER_TYPE_P (TREE_TYPE (new_arg1))
> +	  && int_fits_type_p (arg0, TREE_TYPE (new_arg1)))
> +	new_arg0 = fold_convert (TREE_TYPE (new_arg1), arg0);
> +      else
> +	return false;
> +    }
> +
> +  /* If arg1 is an INTEGER_CST, fold it to new type.  */
> +  if (TREE_CODE (arg1) != SSA_NAME)
> +    {
> +      if (!POINTER_TYPE_P (TREE_TYPE (new_arg0))
> +	  && int_fits_type_p (arg1, TREE_TYPE (new_arg0)))
> +	new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);
> +      else
> +	return false;
> +    }
> +
> +  /* If types of new_arg0 and new_arg1 are different bailout.  */
> +  if (TREE_TYPE (new_arg0) != TREE_TYPE (new_arg1))
> +    return false;
> +
> +  /* Replace the PHI stmt with the new_arg0 and new_arg1.  Also insert
> +     a new CONVER_STMT that converts the phi results.  */

s/CONVER_STMT/CONVERT_STMT/

> +  gsi = gsi_after_labels (gimple_bb (phi));
> +  result = PHI_RESULT (phi);
> +  temp = make_ssa_name (TREE_TYPE (new_arg0), 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 CONVERT_EXPR out from COND_EXPR.\n");
> +      fprintf (dump_file, "New PHI_RESULT is ");
> +      print_generic_expr (dump_file, temp, 0);
> +      fprintf (dump_file, " and new stmt with CONVERT_EXPR defines ");
> +      print_generic_expr (dump_file, result, 0);
> +      fprintf (dump_file, ".\n");
> +    }
> +
> +  gimple_phi_set_result (phi, temp);
> +  SET_PHI_ARG_DEF (phi, e0->dest_idx, new_arg0);
> +  SET_PHI_ARG_DEF (phi, e1->dest_idx, new_arg1);
> +  new_stmt = gimple_build_assign (result, CONVERT_EXPR, temp);
> +  gsi_insert_before (&gsi, new_stmt, GSI_SAME_STMT);
> +  return true;
> +}
> +
>  /*  The function conditional_replacement does the main work of doing the
>      conditional replacement.  Return true if the replacement is done.
>      Otherwise return false.
> @@ -2144,6 +2249,26 @@ gate_hoist_loads (void)
>     This pass also performs a fifth transformation of a slightly different
>     flavor.
>  
> +   Factor conversion in COND_EXPR
> +   ----------------------------------

excess trailing "----" ;)

thanks,
> +
> +   This transformation factors the conversion out of COND_EXPR with
> +   factor_out_conditional_conversion.
> +
> +   For example:
> +   if (a <= CST) goto <bb 3>; else goto <bb 4>;
> +   <bb 3>:
> +   tmp = (int) a;
> +   <bb 4>:
> +   tmp = PHI <tmp, CST>
> +
> +   Into:
> +   if (a <= CST) goto <bb 3>; else goto <bb 4>;
> +   <bb 3>:
> +   <bb 4>:
> +   a = PHI <a, CST>
> +   tmp = (int) a;
> +
>     Adjacent Load Hoisting
>     ----------------------
>