diff mbox

[PR66726] Factor conversion out of COND_EXPR

Message ID 5597D24B.8010900@linaro.org
State New
Headers show

Commit Message

Kugan Vivekanandarajah July 4, 2015, 12:32 p.m. UTC 
On 04/07/15 18:51, Bernhard Reutner-Fischer wrote:
> 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.

Thanks for the comments. Please see the updated patch with the fixes.
Kugan

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

Comments

Bernhard Reutner-Fischer July 4, 2015, 1:21 p.m. UTC | #1 
On July 4, 2015 2:32:11 PM GMT+02:00, Kugan <kugan.vivekanandarajah@linaro.org> wrote:
>On 04/07/15 18:51, Bernhard Reutner-Fischer wrote:
>> 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.

I meant in addition, just to be sure.

Patch itself looks plausible to me but I cannot approve it.

Thanks,
>
>Thanks for the comments. Please see the updated patch with the fixes.
>Kugan
>
>> 
>>>
>>> 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
>>>     ----------------------
>>>  
>>
Jeff Law July 6, 2015, 9:37 p.m. UTC | #2 
On 07/04/2015 06:32 AM, Kugan wrote:

>
> p.txt
>
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c b/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c
> index e69de29..93f1ace 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c
> @@ -0,0 +1,13 @@
> +
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-phiopt1-details" } */
> +
> +extern unsigned short mode_size[];
> +int
> +oof (int mode)
> +{
> +  return (64 < mode_size[mode] ? 64 : mode_size[mode]);
> +}
> +
> +/* { dg-final { scan-tree-dump-times "factor CONVERT_EXPR out" 1 "phiopt1" } } */
I would also verify that this turns into a MIN_EXPR.  I think the patch 
as-written won't detect the MIN_EXPR until the _next_ time phi-opt is 
called.  And one of the benefits we're really looking for here is to 
remove barriers to finding these min/max expressions.


> +
> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> index d2a5cee..12ab9ee 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,
> @@ -342,6 +343,8 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads)
>   	    cfgchanged = true;
>   	  else if (minmax_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
>   	    cfgchanged = true;
> +	  else if (factor_out_conditional_conversion (e1, e2, phi, arg0, arg1))
> +	    cfgchanged = true;
So this transformation does not inherently change the CFG, so setting 
CFGCHANGED isn't really appropriate and may trigger unnecessary cleanups.

I think the transformation needs to occur prior this if-elseif-else 
block since the transformation should enable the code in the 
if-elseif-else block to find more optimization opportunities.

That will also imply we either restart after the transformation applies, 
or we update the local variables that are used as arguments to 
conditional_replacement, abs_replacement and minmax_replacement.


>   	}
>       }
>
> @@ -410,6 +413,108 @@ 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 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 arguments has to be SSA_NAME and other argument can
> +     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))
> +    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 you're just handling two arguments, then it's probably easiest to 
just swap arg0/arg1 e0/e1 if arg0 is not an SSA_NAME like this:

  /* First canonicalize to simplify tests.  */
   if (TREE_CODE (arg0) != SSA_NAME)
     {
       std::swap (arg0, arg1);
       std::swap (e0, e1);
     }

   if (TREE_CODE (arg0) != SSA_NAME)
     return false;

That simplifies things a bit since you're going to know from thsi point 
forward that arg0 is an SSA_NAME.



> +
> +  /* If arg0 is an SSA_NAME and the stmt which defines arg0 is
> +     a CONVERT_STMT, use the LHS as new_arg0.  */
> +  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);
> +    }
Use gimple_assign_cast_p rather than checking CONVERT_EXPR_CODE_P 
directly, so something like:

   /* Now see if ARG0 was defined by a typecast.  */
   gimple arg0_def = SSA_NAME_DEF_STMT (arg0);

   if (!is_gimple_assign (arg0_def) || !gimple_assign_cast_p (arg0_def))
     return false;

Similarly for arg1 when it's an SSA_NAME.


> +
> +  /* If types of new_arg0 and new_arg1 are different bailout.  */
> +  if (TREE_TYPE (new_arg0) != TREE_TYPE (new_arg1))
> +    return false;
Do we want to restrict this to just integral types?    I haven't though 
about it too deeply, so perhaps not.

> +
> +  /* Replace the PHI stmt with the new_arg0 and new_arg1.  Also insert
> +     a new CONVERT_STMT that converts the phi results.  */
> +  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;
So I think you want to also remove the old cast(s) so that the minmax 
optimization can apply without having to wait for another pass of phi-opt.

To safely remove the old cast(s) you have to verify the result of the 
cast has a single use (which is obviously in the PHI).  Otherwise your 
transformation will have introduced a runtime redundancy.

I also suspect it's better to create a new PHI rather than modify the 
original PHI.  The original PHI should be removed and the result re-used 
as the result of the new convert statement.

Extra points if you can easily make this transformation apply to a 
generic unary operator.  So for example, we might have a sinkable bit-not.

Overall it's heading the right direction.  But I think it needs another 
iteration.

jeff
diff mbox

Patch

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c b/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c
index e69de29..93f1ace 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c
@@ -0,0 +1,13 @@ 
+
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-phiopt1-details" } */
+
+extern unsigned short mode_size[];
+int
+oof (int mode)
+{
+  return (64 < mode_size[mode] ? 64 : mode_size[mode]);
+}
+
+/* { dg-final { scan-tree-dump-times "factor CONVERT_EXPR out" 1 "phiopt1" } } */
+
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index d2a5cee..12ab9ee 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,
@@ -342,6 +343,8 @@  tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads)
 	    cfgchanged = true;
 	  else if (minmax_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
 	    cfgchanged = true;
+	  else if (factor_out_conditional_conversion (e1, e2, phi, arg0, arg1))
+	    cfgchanged = true;
 	}
     }
 
@@ -410,6 +413,108 @@  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 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 arguments has to be SSA_NAME and other argument can
+     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))
+    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
+     a CONVERT_STMT, use the LHS as new_arg0.  */
+  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
+     a CONVERT_STMT, use the LHS as new_arg1.  */
+  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 CONVERT_STMT that converts the phi results.  */
+  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
+   ------------------------------
+
+   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
    ----------------------