* [PR66726] Factor conversion out of COND_EXPR
@ 2015-07-04 2:59 Kugan
2015-07-04 8:51 ` Bernhard Reutner-Fischer
0 siblings, 1 reply; 16+ messages in thread
From: Kugan @ 2015-07-04 2:59 UTC (permalink / raw)
To: gcc-patches; +Cc: Jeff Law
[-- Attachment #1: Type: text/plain, Size: 648 bytes --]
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.
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.
[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 5935 bytes --]
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c b/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c
index e69de29..b636c8f 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-phiopt2" } */
+
+extern unsigned short mode_size[];
+int
+oof (int mode)
+{
+ return (64 < mode_size[mode] ? 64 : mode_size[mode]);
+}
+
+/* { dg-final { scan-tree-dump-times "MIN_EXPR" 1 "phiopt2" } } */
+
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
@@ -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 argument 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 argument 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
+ ai 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
+ ai 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 CONVER_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
----------------------
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PR66726] Factor conversion out of COND_EXPR
2015-07-04 2:59 [PR66726] Factor conversion out of COND_EXPR Kugan
@ 2015-07-04 8:51 ` Bernhard Reutner-Fischer
2015-07-04 12:32 ` Kugan
0 siblings, 1 reply; 16+ messages in thread
From: Bernhard Reutner-Fischer @ 2015-07-04 8:51 UTC (permalink / raw)
To: Kugan; +Cc: gcc-patches, Jeff Law
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
> ----------------------
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PR66726] Factor conversion out of COND_EXPR
2015-07-04 8:51 ` Bernhard Reutner-Fischer
@ 2015-07-04 12:32 ` Kugan
2015-07-04 13:21 ` Bernhard Reutner-Fischer
2015-07-06 21:37 ` Jeff Law
0 siblings, 2 replies; 16+ messages in thread
From: Kugan @ 2015-07-04 12:32 UTC (permalink / raw)
To: Bernhard Reutner-Fischer; +Cc: gcc-patches, Jeff Law
[-- Attachment #1: Type: text/plain, Size: 6382 bytes --]
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
>> ----------------------
>>
>
[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 5955 bytes --]
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
----------------------
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PR66726] Factor conversion out of COND_EXPR
2015-07-04 12:32 ` Kugan
@ 2015-07-04 13:21 ` Bernhard Reutner-Fischer
2015-07-06 21:37 ` Jeff Law
1 sibling, 0 replies; 16+ messages in thread
From: Bernhard Reutner-Fischer @ 2015-07-04 13:21 UTC (permalink / raw)
To: Kugan; +Cc: gcc-patches, Jeff Law
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
>>> ----------------------
>>>
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PR66726] Factor conversion out of COND_EXPR
2015-07-04 12:32 ` Kugan
2015-07-04 13:21 ` Bernhard Reutner-Fischer
@ 2015-07-06 21:37 ` Jeff Law
2015-07-07 12:50 ` Kugan
1 sibling, 1 reply; 16+ messages in thread
From: Jeff Law @ 2015-07-06 21:37 UTC (permalink / raw)
To: Kugan, Bernhard Reutner-Fischer; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PR66726] Factor conversion out of COND_EXPR
2015-07-06 21:37 ` Jeff Law
@ 2015-07-07 12:50 ` Kugan
2015-07-07 14:40 ` Jeff Law
0 siblings, 1 reply; 16+ messages in thread
From: Kugan @ 2015-07-07 12:50 UTC (permalink / raw)
To: Jeff Law, Bernhard Reutner-Fischer; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 7161 bytes --]
On 07/07/15 07:37, Jeff Law wrote:
> On 07/04/2015 06:32 AM, Kugan wrote:
> 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.
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?
Bootstrapped and regression tested the attached patch on
x86-64-none-linux-gnu with no new regressions.
Thanks,
Kugan
[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 7033 bytes --]
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c b/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c
index e69de29..a4c7418 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c
@@ -0,0 +1,15 @@
+
+/* { 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 conversion out" 1 "phiopt1" } } */
+/* { dg-final { scan-tree-dump-times "MIN_EXPR" 1 "phiopt1" } } */
+
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);
+ }
+
/* 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;
+
+ /* 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
+ || (TREE_CODE (arg1) != SSA_NAME
+ && TREE_CODE (arg1) != INTEGER_CST))
+ return false;
+
+ /* 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)))
+ return false;
+
+ /* Use the LHS as new_arg0. */
+ 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);
+
+ /* If arg1 is an SSA_NAME and the stmt which defines arg0 is
+ a conversion. */
+ if (TREE_CODE (arg1) == SSA_NAME)
+ {
+ arg1_def_stmt = SSA_NAME_DEF_STMT (arg1);
+ if (!is_gimple_assign (arg1_def_stmt)
+ || gimple_assign_rhs_code (arg1_def_stmt) != convert_code)
+ return false;
+
+ /* Use the LHS as new_arg1. */
+ new_arg1 = gimple_assign_rhs1 (arg1_def_stmt);
+ if (gimple_assign_rhs_code (arg1_def_stmt) == VIEW_CONVERT_EXPR)
+ new_arg1 = TREE_OPERAND (new_arg1, 0);
+ }
+ else
+ {
+ /* If arg1 is an INTEGER_CST, fold it to new type. */
+ if (INTEGRAL_TYPE_P (TREE_TYPE (new_arg0))
+ && 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);
+ else
+ return false;
+ }
+ 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;
+
+ /* 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);
+ }
+
+ add_phi_arg (newphi, new_arg0, e0, locus);
+ add_phi_arg (newphi, new_arg1, e1, locus);
+
+ /* Create the conversion stmt and insert it. */
+ if (convert_code == VIEW_CONVERT_EXPR)
+ temp = fold_build1 (VIEW_CONVERT_EXPR, TREE_TYPE (result), temp);
+ new_stmt = gimple_build_assign (result, convert_code, temp);
+ gsi = gsi_after_labels (gimple_bb (phi));
+ gsi_insert_before (&gsi, new_stmt, GSI_SAME_STMT);
+
+ /* Remove he original PHI stmt. */
+ gsi = gsi_for_stmt (phi);
+ gsi_remove (&gsi, true);
+ 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.
@@ -2142,6 +2281,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
----------------------
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PR66726] Factor conversion out of COND_EXPR
2015-07-07 12:50 ` Kugan
@ 2015-07-07 14:40 ` Jeff Law
2015-07-09 23:08 ` Kugan
0 siblings, 1 reply; 16+ messages in thread
From: Jeff Law @ 2015-07-07 14:40 UTC (permalink / raw)
To: Kugan, Bernhard Reutner-Fischer; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PR66726] Factor conversion out of COND_EXPR
2015-07-07 14:40 ` Jeff Law
@ 2015-07-09 23:08 ` Kugan
2015-07-10 20:40 ` Jeff Law
0 siblings, 1 reply; 16+ messages in thread
From: Kugan @ 2015-07-09 23:08 UTC (permalink / raw)
To: Jeff Law, Bernhard Reutner-Fischer; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 7791 bytes --]
On 08/07/15 00:41, Jeff Law wrote:
> 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.
Thanks Jeff for the review.As you said later, I will skip generic unary
in this patch and work on that as an addition on top of this.
>
>>
>> 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.
Done.
>
>
>> /* 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.
>
Comment added. I will work on the implementation after this patch.
>> +
>> + /* 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/
>
Done.
>> + 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?
>
I experimented with this and it seems to me that FIX_TRUNC_EXPR does not
need this. I added an execution testcase for VIEW_CONVERT_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.
Done.
>
> 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.
>
Done. Bootstrapped and regression tested on x86-64-none-linux-gnu with
no new regressions. Is this OK for trunk?
Thanks,
Kugan
[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 8591 bytes --]
diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr66726.c b/gcc/testsuite/g++.dg/tree-ssa/pr66726.c
index e69de29..9b3bd8f 100644
--- a/gcc/testsuite/g++.dg/tree-ssa/pr66726.c
+++ b/gcc/testsuite/g++.dg/tree-ssa/pr66726.c
@@ -0,0 +1,36 @@
+
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+/* Execution test for converting VIEW_CONVERT_EXPR. */
+
+struct cpp_num {
+ bool f;
+};
+
+extern cpp_num __attribute__((noinline))
+foo (cpp_num lhs,
+ cpp_num rhs)
+{
+ lhs.f = lhs.f || rhs.f;
+ return lhs;
+}
+
+cpp_num lhs, rhs, r;
+
+int main ()
+{
+
+ lhs.f = false;
+ rhs.f = false;
+ r = foo (lhs, rhs);
+ if (r.f)
+ __builtin_abort ();
+
+
+ lhs.f = false;
+ rhs.f = true;
+ r = foo (lhs, rhs);
+ if (!r.f)
+ __builtin_abort ();
+}
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr66726-2.c b/gcc/testsuite/gcc.dg/tree-ssa/pr66726-2.c
index e69de29..ab43d48 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr66726-2.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr66726-2.c
@@ -0,0 +1,19 @@
+
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-phiopt1-details" } */
+
+extern void bar (char, char);
+int
+foo (char b)
+{
+ char a;
+ a = b;
+ b = 'b';
+ bar (a, b);
+ b = a;
+ if (b == 0)
+ a++;
+ return a + b;
+}
+
+/* { dg-final { scan-tree-dump-times "factor conversion out" 0 "phiopt1" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c b/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c
index e69de29..a4c7418 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr66726.c
@@ -0,0 +1,15 @@
+
+/* { 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 conversion out" 1 "phiopt1" } } */
+/* { dg-final { scan-tree-dump-times "MIN_EXPR" 1 "phiopt1" } } */
+
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index 92b4ab0..766ec63 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,19 @@ 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))
+ {
+ /* 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. */
+ 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);
+ }
+
/* Do the replacement of conditional if it can be done. */
if (conditional_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
cfgchanged = true;
@@ -410,6 +424,134 @@ 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 or if their defining
+ statement have the same unary operation, we can handle more
+ than two arguments too. */
+ if (gimple_phi_num_args (phi) != 2)
+ return false;
+
+ /* 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
+ || (TREE_CODE (arg1) != SSA_NAME
+ && TREE_CODE (arg1) != INTEGER_CST))
+ return false;
+
+ /* Check 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))
+ return false;
+
+ /* Use the RHS as new_arg0. */
+ 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);
+
+ if (TREE_CODE (arg1) == SSA_NAME)
+ {
+ /* Check if arg1 is an SSA_NAME and the stmt which defines arg1
+ is a conversion. */
+ arg1_def_stmt = SSA_NAME_DEF_STMT (arg1);
+ if (!is_gimple_assign (arg1_def_stmt)
+ || gimple_assign_rhs_code (arg1_def_stmt) != convert_code)
+ return false;
+
+ /* Use the RHS as new_arg1. */
+ new_arg1 = gimple_assign_rhs1 (arg1_def_stmt);
+ if (convert_code == VIEW_CONVERT_EXPR)
+ new_arg1 = TREE_OPERAND (new_arg1, 0);
+ }
+ else
+ {
+ /* If arg1 is an INTEGER_CST, fold it to new type. */
+ if (INTEGRAL_TYPE_P (TREE_TYPE (new_arg0))
+ && 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);
+ else
+ return false;
+ }
+ else
+ return false;
+ }
+
+ /* If arg0/arg1 have > 1 use, then this transformation actually increases
+ the number of expressions evaluated at runtime. */
+ if (!has_single_use (arg0)
+ || (arg1_def_stmt && !has_single_use (arg1)))
+ return false;
+
+ /* If types of new_arg0 and new_arg1 are different bailout. */
+ if (TREE_TYPE (new_arg0) != TREE_TYPE (new_arg1))
+ return false;
+
+ /* 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. */
+ gsi_for_def = gsi_for_stmt (arg0_def_stmt);
+ gsi_remove (&gsi_for_def, true);
+ if (arg1_def_stmt)
+ {
+ gsi_for_def = gsi_for_stmt (arg1_def_stmt);
+ gsi_remove (&gsi_for_def, true);
+ }
+
+ add_phi_arg (newphi, new_arg0, e0, locus);
+ add_phi_arg (newphi, new_arg1, e1, locus);
+
+ /* Create the conversion stmt and insert it. */
+ if (convert_code == VIEW_CONVERT_EXPR)
+ temp = fold_build1 (VIEW_CONVERT_EXPR, TREE_TYPE (result), temp);
+ new_stmt = gimple_build_assign (result, convert_code, temp);
+ gsi = gsi_after_labels (gimple_bb (phi));
+ gsi_insert_before (&gsi, new_stmt, GSI_SAME_STMT);
+
+ /* Remove he original PHI stmt. */
+ gsi = gsi_for_stmt (phi);
+ gsi_remove (&gsi, true);
+ 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.
@@ -2142,6 +2284,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
----------------------
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PR66726] Factor conversion out of COND_EXPR
2015-07-09 23:08 ` Kugan
@ 2015-07-10 20:40 ` Jeff Law
2015-07-12 11:24 ` Kugan
0 siblings, 1 reply; 16+ messages in thread
From: Jeff Law @ 2015-07-10 20:40 UTC (permalink / raw)
To: Kugan, Bernhard Reutner-Fischer; +Cc: gcc-patches
On 07/09/2015 05:08 PM, Kugan wrote:
> Done. Bootstrapped and regression tested on x86-64-none-linux-gnu with
> no new regressions. Is this OK for trunk?
Thanks for the additional testcases.
> + else
> + {
> + /* If arg1 is an INTEGER_CST, fold it to new type. */
> + if (INTEGRAL_TYPE_P (TREE_TYPE (new_arg0))
> + && 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);
> + else
> + return false;
> + }
> + else
> + return false;
> + }
Something looks goofy here formatting-wise. Can you please check for
horizontal whitespace consistency before committing.
> +
> + /* If types of new_arg0 and new_arg1 are different bailout. */
> + if (TREE_TYPE (new_arg0) != TREE_TYPE (new_arg1))
> + return false;
Seems like this should use types_compatible_p here. You're testing
pointer equality, but as long as the types are compatible, we should be
able to make the transformation.
With the horizontal whitespace fixed and using types_compatible_p this
is OK for the trunk. So pre-approved with those two changes and a final
bootstrap/regression test (due to the types_compatible_p change).
jeff
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PR66726] Factor conversion out of COND_EXPR
2015-07-10 20:40 ` Jeff Law
@ 2015-07-12 11:24 ` Kugan
2015-07-15 7:49 ` Kugan
0 siblings, 1 reply; 16+ messages in thread
From: Kugan @ 2015-07-12 11:24 UTC (permalink / raw)
To: Jeff Law, Bernhard Reutner-Fischer; +Cc: gcc-patches
On 11/07/15 06:40, Jeff Law wrote:
> On 07/09/2015 05:08 PM, Kugan wrote:
>
>> Done. Bootstrapped and regression tested on x86-64-none-linux-gnu with
>> no new regressions. Is this OK for trunk?
> Thanks for the additional testcases.
>
>
>
>> + else
>> + {
>> + /* If arg1 is an INTEGER_CST, fold it to new type. */
>> + if (INTEGRAL_TYPE_P (TREE_TYPE (new_arg0))
>> + && 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);
>> + else
>> + return false;
>> + }
>> + else
>> + return false;
>> + }
> Something looks goofy here formatting-wise. Can you please check for
> horizontal whitespace consistency before committing.
>
>
>
>> +
>> + /* If types of new_arg0 and new_arg1 are different bailout. */
>> + if (TREE_TYPE (new_arg0) != TREE_TYPE (new_arg1))
>> + return false;
> Seems like this should use types_compatible_p here. You're testing
> pointer equality, but as long as the types are compatible, we should be
> able to make the transformation.
>
> With the horizontal whitespace fixed and using types_compatible_p this
> is OK for the trunk. So pre-approved with those two changes and a final
> bootstrap/regression test (due to the types_compatible_p change).
>
> jeff
>
Thanks. Committed as r225722 with the changes. Also did a fresh
bootstrap and regression testing on x86_64-none-linux-gnu before committing.
Thanks,
Kugan
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PR66726] Factor conversion out of COND_EXPR
2015-07-12 11:24 ` Kugan
@ 2015-07-15 7:49 ` Kugan
2015-07-15 18:55 ` Jeff Law
0 siblings, 1 reply; 16+ messages in thread
From: Kugan @ 2015-07-15 7:49 UTC (permalink / raw)
To: Jeff Law, Bernhard Reutner-Fischer; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 639 bytes --]
Here is a patch to fix to teach tree-ssa-reassoc the sinking the cast.
Bootstrapped and regression tested on x86-64-none-linux-gnu with no new
regressions. Also regression tested on qemu arm.
I also verified the issue Andreas Schwab raised is fixed on arm
cortex-a5 where the same issue was present. Does this make sense?
Thanks,
Kugan
gcc/ChangeLog:
2015-07-15 Kugan Vivekanandarajah <kuganv@linaro.org>
PR middle-end/66726
* tree-ssa-reassoc.c (optimize_range_tests): Handle sinking the cast
after PHI.
(final_range_test_p): Detect sinking the cast after PHI.
(maybe_optimize_range_tests): Handle sinking the cast after PHI.
[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 4007 bytes --]
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index 932c83a..3058eb5 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -2707,18 +2707,32 @@ optimize_range_tests (enum tree_code opcode,
# _345 = PHI <_123(N), 1(...), 1(...)>
where _234 has bool type, _123 has single use and
bb N has a single successor M. This is commonly used in
- the last block of a range test. */
+ the last block of a range test.
+
+ Also Return true if STMT is tcc_compare like:
+ <bb N>:
+ ...
+ _234 = a_2(D) == 2;
+ <bb M>:
+ # _345 = PHI <_234(N), 1(...), 1(...)>
+ _346 = (int) _345;
+ where _234 has booltype, single use and
+ bb N has a single successor M. This is commonly used in
+ the last block of a range test. */
static bool
final_range_test_p (gimple stmt)
{
- basic_block bb, rhs_bb;
+ basic_block bb, rhs_bb, lhs_bb;
edge e;
tree lhs, rhs;
use_operand_p use_p;
gimple use_stmt;
- if (!gimple_assign_cast_p (stmt))
+ if (!gimple_assign_cast_p (stmt)
+ && (!is_gimple_assign (stmt)
+ || (TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
+ != tcc_comparison)))
return false;
bb = gimple_bb (stmt);
if (!single_succ_p (bb))
@@ -2729,9 +2743,8 @@ final_range_test_p (gimple stmt)
lhs = gimple_assign_lhs (stmt);
rhs = gimple_assign_rhs1 (stmt);
- if (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
- || TREE_CODE (rhs) != SSA_NAME
- || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE)
+ if (TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE
+ && TREE_CODE (TREE_TYPE (lhs)) != BOOLEAN_TYPE)
return false;
/* Test whether lhs is consumed only by a PHI in the only successor bb. */
@@ -2743,10 +2756,21 @@ final_range_test_p (gimple stmt)
return false;
/* And that the rhs is defined in the same loop. */
- rhs_bb = gimple_bb (SSA_NAME_DEF_STMT (rhs));
- if (rhs_bb == NULL
- || !flow_bb_inside_loop_p (loop_containing_stmt (stmt), rhs_bb))
- return false;
+ if (gimple_assign_cast_p (stmt))
+ {
+ if (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
+ || TREE_CODE (rhs) != SSA_NAME
+ || !(rhs_bb = gimple_bb (SSA_NAME_DEF_STMT (rhs)))
+ || !flow_bb_inside_loop_p (loop_containing_stmt (stmt), rhs_bb))
+ return false;
+ }
+ else
+ {
+ if (TREE_CODE (lhs) != SSA_NAME
+ || !(lhs_bb = gimple_bb (SSA_NAME_DEF_STMT (lhs)))
+ || !flow_bb_inside_loop_p (loop_containing_stmt (stmt), lhs_bb))
+ return false;
+ }
return true;
}
@@ -3132,6 +3156,8 @@ maybe_optimize_range_tests (gimple stmt)
/* stmt is
_123 = (int) _234;
+ OR
+ _234 = a_2(D) == 2;
followed by:
<bb M>:
@@ -3161,6 +3187,8 @@ maybe_optimize_range_tests (gimple stmt)
of the bitwise or resp. and, recursively. */
if (!get_ops (rhs, code, &ops,
loop_containing_stmt (stmt))
+ && (TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
+ != tcc_comparison)
&& has_single_use (rhs))
{
/* Otherwise, push the _234 range test itself. */
@@ -3173,6 +3201,22 @@ maybe_optimize_range_tests (gimple stmt)
ops.safe_push (oe);
bb_ent.last_idx++;
}
+ else if (!get_ops (lhs, code, &ops,
+ loop_containing_stmt (stmt))
+ && is_gimple_assign (stmt)
+ && (TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
+ == tcc_comparison)
+ && has_single_use (lhs))
+ {
+ /* Push the _234 range test itself. */
+ operand_entry_t oe = operand_entry_pool.allocate ();
+ oe->op = lhs;
+ oe->rank = code;
+ oe->id = 0;
+ oe->count = 1;
+ ops.safe_push (oe);
+ bb_ent.last_idx++;
+ }
else
bb_ent.last_idx = ops.length ();
bb_ent.op = rhs;
@@ -3267,7 +3311,8 @@ maybe_optimize_range_tests (gimple stmt)
else if (gimple_assign_cast_p (use_stmt))
cast_stmt = use_stmt;
else
- gcc_unreachable ();
+ cast_stmt = NULL;
+
if (cast_stmt)
{
gcc_assert (bb == last_bb);
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PR66726] Factor conversion out of COND_EXPR
2015-07-15 7:49 ` Kugan
@ 2015-07-15 18:55 ` Jeff Law
2015-07-16 7:06 ` Kugan
0 siblings, 1 reply; 16+ messages in thread
From: Jeff Law @ 2015-07-15 18:55 UTC (permalink / raw)
To: Kugan, Bernhard Reutner-Fischer; +Cc: gcc-patches
On 07/15/2015 01:09 AM, Kugan wrote:
>
> 2015-07-15 Kugan Vivekanandarajah<kuganv@linaro.org>
>
> PR middle-end/66726
> * tree-ssa-reassoc.c (optimize_range_tests): Handle sinking the cast
> after PHI.
> (final_range_test_p): Detect sinking the cast after PHI.
> (maybe_optimize_range_tests): Handle sinking the cast after PHI.
Can we tweak
>
>
> p.txt
>
>
> diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
> index 932c83a..3058eb5 100644
> --- a/gcc/tree-ssa-reassoc.c
> +++ b/gcc/tree-ssa-reassoc.c
> return false;
> bb = gimple_bb (stmt);
> if (!single_succ_p (bb))
> @@ -2729,9 +2743,8 @@ final_range_test_p (gimple stmt)
>
> lhs = gimple_assign_lhs (stmt);
> rhs = gimple_assign_rhs1 (stmt);
> - if (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
> - || TREE_CODE (rhs) != SSA_NAME
> - || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE)
> + if (TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE
> + && TREE_CODE (TREE_TYPE (lhs)) != BOOLEAN_TYPE)
> return false;
So you're ensuring that one of the two is a boolean... Note that
previously we ensured that the rhs was a boolean and the lhs was an
integral type (which I believe is true for booleans).
Thus if we had
bool x;
int y;
x = (bool) y;
The old code would have rejected that case. But I think it gets through
now, right?
I think once that issue is addressed, this will be good for the trunk.
jeff
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PR66726] Factor conversion out of COND_EXPR
2015-07-15 18:55 ` Jeff Law
@ 2015-07-16 7:06 ` Kugan
2015-07-23 19:48 ` Jeff Law
0 siblings, 1 reply; 16+ messages in thread
From: Kugan @ 2015-07-16 7:06 UTC (permalink / raw)
To: Jeff Law, Bernhard Reutner-Fischer; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1721 bytes --]
>>
>> diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
>> index 932c83a..3058eb5 100644
>> --- a/gcc/tree-ssa-reassoc.c
>> +++ b/gcc/tree-ssa-reassoc.c
>
>> return false;
>> bb = gimple_bb (stmt);
>> if (!single_succ_p (bb))
>> @@ -2729,9 +2743,8 @@ final_range_test_p (gimple stmt)
>>
>> lhs = gimple_assign_lhs (stmt);
>> rhs = gimple_assign_rhs1 (stmt);
>> - if (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
>> - || TREE_CODE (rhs) != SSA_NAME
>> - || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE)
>> + if (TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE
>> + && TREE_CODE (TREE_TYPE (lhs)) != BOOLEAN_TYPE)
>> return false;
> So you're ensuring that one of the two is a boolean... Note that
> previously we ensured that the rhs was a boolean and the lhs was an
> integral type (which I believe is true for booleans).
>
> Thus if we had
> bool x;
> int y;
>
> x = (bool) y;
>
> The old code would have rejected that case. But I think it gets through
> now, right?
>
> I think once that issue is addressed, this will be good for the trunk.
>
Thanks for the review. How about:
- if (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
- || TREE_CODE (rhs) != SSA_NAME
- || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE)
+ if (gimple_assign_cast_p (stmt)
+ && (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
+ || TREE_CODE (rhs) != SSA_NAME
+ || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE))
Thanks,
Kugan
gcc/ChangeLog:
2015-07-16 Kugan Vivekanandarajah <kuganv@linaro.org>
PR middle-end/66726
* tree-ssa-reassoc.c (optimize_range_tests): Handle tcc_compare stmt
whose result is used in PHI.
(maybe_optimize_range_tests): Likewise.
(final_range_test_p): Lokweise.
[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 4029 bytes --]
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index 932c83a..78c80d6 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -2707,18 +2707,32 @@ optimize_range_tests (enum tree_code opcode,
# _345 = PHI <_123(N), 1(...), 1(...)>
where _234 has bool type, _123 has single use and
bb N has a single successor M. This is commonly used in
- the last block of a range test. */
+ the last block of a range test.
+
+ Also Return true if STMT is tcc_compare like:
+ <bb N>:
+ ...
+ _234 = a_2(D) == 2;
+ <bb M>:
+ # _345 = PHI <_234(N), 1(...), 1(...)>
+ _346 = (int) _345;
+ where _234 has booltype, single use and
+ bb N has a single successor M. This is commonly used in
+ the last block of a range test. */
static bool
final_range_test_p (gimple stmt)
{
- basic_block bb, rhs_bb;
+ basic_block bb, rhs_bb, lhs_bb;
edge e;
tree lhs, rhs;
use_operand_p use_p;
gimple use_stmt;
- if (!gimple_assign_cast_p (stmt))
+ if (!gimple_assign_cast_p (stmt)
+ && (!is_gimple_assign (stmt)
+ || (TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
+ != tcc_comparison)))
return false;
bb = gimple_bb (stmt);
if (!single_succ_p (bb))
@@ -2729,9 +2743,10 @@ final_range_test_p (gimple stmt)
lhs = gimple_assign_lhs (stmt);
rhs = gimple_assign_rhs1 (stmt);
- if (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
- || TREE_CODE (rhs) != SSA_NAME
- || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE)
+ if (gimple_assign_cast_p (stmt)
+ && (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
+ || TREE_CODE (rhs) != SSA_NAME
+ || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE))
return false;
/* Test whether lhs is consumed only by a PHI in the only successor bb. */
@@ -2743,10 +2758,20 @@ final_range_test_p (gimple stmt)
return false;
/* And that the rhs is defined in the same loop. */
- rhs_bb = gimple_bb (SSA_NAME_DEF_STMT (rhs));
- if (rhs_bb == NULL
- || !flow_bb_inside_loop_p (loop_containing_stmt (stmt), rhs_bb))
- return false;
+ if (gimple_assign_cast_p (stmt))
+ {
+ if (TREE_CODE (rhs) != SSA_NAME
+ || !(rhs_bb = gimple_bb (SSA_NAME_DEF_STMT (rhs)))
+ || !flow_bb_inside_loop_p (loop_containing_stmt (stmt), rhs_bb))
+ return false;
+ }
+ else
+ {
+ if (TREE_CODE (lhs) != SSA_NAME
+ || !(lhs_bb = gimple_bb (SSA_NAME_DEF_STMT (lhs)))
+ || !flow_bb_inside_loop_p (loop_containing_stmt (stmt), lhs_bb))
+ return false;
+ }
return true;
}
@@ -3132,6 +3157,8 @@ maybe_optimize_range_tests (gimple stmt)
/* stmt is
_123 = (int) _234;
+ OR
+ _234 = a_2(D) == 2;
followed by:
<bb M>:
@@ -3161,6 +3188,8 @@ maybe_optimize_range_tests (gimple stmt)
of the bitwise or resp. and, recursively. */
if (!get_ops (rhs, code, &ops,
loop_containing_stmt (stmt))
+ && (TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
+ != tcc_comparison)
&& has_single_use (rhs))
{
/* Otherwise, push the _234 range test itself. */
@@ -3173,6 +3202,22 @@ maybe_optimize_range_tests (gimple stmt)
ops.safe_push (oe);
bb_ent.last_idx++;
}
+ else if (!get_ops (lhs, code, &ops,
+ loop_containing_stmt (stmt))
+ && is_gimple_assign (stmt)
+ && (TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
+ == tcc_comparison)
+ && has_single_use (lhs))
+ {
+ /* Push the _234 range test itself. */
+ operand_entry_t oe = operand_entry_pool.allocate ();
+ oe->op = lhs;
+ oe->rank = code;
+ oe->id = 0;
+ oe->count = 1;
+ ops.safe_push (oe);
+ bb_ent.last_idx++;
+ }
else
bb_ent.last_idx = ops.length ();
bb_ent.op = rhs;
@@ -3267,7 +3312,8 @@ maybe_optimize_range_tests (gimple stmt)
else if (gimple_assign_cast_p (use_stmt))
cast_stmt = use_stmt;
else
- gcc_unreachable ();
+ cast_stmt = NULL;
+
if (cast_stmt)
{
gcc_assert (bb == last_bb);
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PR66726] Factor conversion out of COND_EXPR
2015-07-16 7:06 ` Kugan
@ 2015-07-23 19:48 ` Jeff Law
2015-07-27 3:10 ` Kugan
0 siblings, 1 reply; 16+ messages in thread
From: Jeff Law @ 2015-07-23 19:48 UTC (permalink / raw)
To: Kugan, Bernhard Reutner-Fischer; +Cc: gcc-patches
On 07/15/2015 11:52 PM, Kugan wrote:
>
>>>
>>> diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
>>> index 932c83a..3058eb5 100644
>>> --- a/gcc/tree-ssa-reassoc.c
>>> +++ b/gcc/tree-ssa-reassoc.c
>>
>>> return false;
>>> bb = gimple_bb (stmt);
>>> if (!single_succ_p (bb))
>>> @@ -2729,9 +2743,8 @@ final_range_test_p (gimple stmt)
>>>
>>> lhs = gimple_assign_lhs (stmt);
>>> rhs = gimple_assign_rhs1 (stmt);
>>> - if (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
>>> - || TREE_CODE (rhs) != SSA_NAME
>>> - || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE)
>>> + if (TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE
>>> + && TREE_CODE (TREE_TYPE (lhs)) != BOOLEAN_TYPE)
>>> return false;
>> So you're ensuring that one of the two is a boolean... Note that
>> previously we ensured that the rhs was a boolean and the lhs was an
>> integral type (which I believe is true for booleans).
>>
>> Thus if we had
>> bool x;
>> int y;
>>
>> x = (bool) y;
>>
>> The old code would have rejected that case. But I think it gets through
>> now, right?
>>
>> I think once that issue is addressed, this will be good for the trunk.
>>
>
> Thanks for the review. How about:
>
> - if (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
> - || TREE_CODE (rhs) != SSA_NAME
> - || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE)
> + if (gimple_assign_cast_p (stmt)
> + && (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
> + || TREE_CODE (rhs) != SSA_NAME
> + || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE))
But then I think you need to verify that for the _234 = a_2(D) == 2;
case that type of the RHS is a boolean.
ie, each case has requirements for the types. I don't think they can be
reasonably unified. So something like this:
if (gimple_assign_cast_p (stmt)
&& ! (correct types for cast)
return false;
if (!gimple_assign_cast_p (stmt)
&& ! (correct types for tcc_comparison case))
return false;
This works because we've already verified that it's either a type
conversion or a comparison on the RHS.
Jeff
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PR66726] Factor conversion out of COND_EXPR
2015-07-23 19:48 ` Jeff Law
@ 2015-07-27 3:10 ` Kugan
2015-08-03 16:59 ` Jeff Law
0 siblings, 1 reply; 16+ messages in thread
From: Kugan @ 2015-07-27 3:10 UTC (permalink / raw)
To: Jeff Law, Bernhard Reutner-Fischer; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2640 bytes --]
On 24/07/15 05:05, Jeff Law wrote:
> On 07/15/2015 11:52 PM, Kugan wrote:
>>
>>>>
>>>> diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
>>>> index 932c83a..3058eb5 100644
>>>> --- a/gcc/tree-ssa-reassoc.c
>>>> +++ b/gcc/tree-ssa-reassoc.c
>>>
>>>> return false;
>>>> bb = gimple_bb (stmt);
>>>> if (!single_succ_p (bb))
>>>> @@ -2729,9 +2743,8 @@ final_range_test_p (gimple stmt)
>>>>
>>>> lhs = gimple_assign_lhs (stmt);
>>>> rhs = gimple_assign_rhs1 (stmt);
>>>> - if (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
>>>> - || TREE_CODE (rhs) != SSA_NAME
>>>> - || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE)
>>>> + if (TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE
>>>> + && TREE_CODE (TREE_TYPE (lhs)) != BOOLEAN_TYPE)
>>>> return false;
>>> So you're ensuring that one of the two is a boolean... Note that
>>> previously we ensured that the rhs was a boolean and the lhs was an
>>> integral type (which I believe is true for booleans).
>>>
>>> Thus if we had
>>> bool x;
>>> int y;
>>>
>>> x = (bool) y;
>>>
>>> The old code would have rejected that case. But I think it gets through
>>> now, right?
>>>
>>> I think once that issue is addressed, this will be good for the trunk.
>>>
>>
>> Thanks for the review. How about:
>>
>> - if (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
>> - || TREE_CODE (rhs) != SSA_NAME
>> - || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE)
>> + if (gimple_assign_cast_p (stmt)
>> + && (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
>> + || TREE_CODE (rhs) != SSA_NAME
>> + || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE))
> But then I think you need to verify that for the _234 = a_2(D) == 2;
> case that type of the RHS is a boolean.
>
> ie, each case has requirements for the types. I don't think they can be
> reasonably unified. So something like this:
>
> if (gimple_assign_cast_p (stmt)
> && ! (correct types for cast)
> return false;
>
> if (!gimple_assign_cast_p (stmt)
> && ! (correct types for tcc_comparison case))
> return false;
>
>
> This works because we've already verified that it's either a type
> conversion or a comparison on the RHS.
>
I thought that when !gimple_assign_cast_p (stmt), RHS will always
boolean. I have now added this check in the attached patch.
I also noticed that in maybe_optimize_range_tests, GIMPLE_COND can
have non compatible types when new_op is updated
(boolean types coming from tcc_compare results) and hence need to be
converted. Changed that as well.
Bootstrapped and regression tested on x86-64-none-linux-gnu with no new
regressions. Is this OK for trunk?
Thanks,
Kugan
[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 6185 bytes --]
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index efb813c..cc215b6 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -2707,18 +2707,32 @@ optimize_range_tests (enum tree_code opcode,
# _345 = PHI <_123(N), 1(...), 1(...)>
where _234 has bool type, _123 has single use and
bb N has a single successor M. This is commonly used in
- the last block of a range test. */
+ the last block of a range test.
+
+ Also Return true if STMT is tcc_compare like:
+ <bb N>:
+ ...
+ _234 = a_2(D) == 2;
+ <bb M>:
+ # _345 = PHI <_234(N), 1(...), 1(...)>
+ _346 = (int) _345;
+ where _234 has booltype, single use and
+ bb N has a single successor M. This is commonly used in
+ the last block of a range test. */
static bool
final_range_test_p (gimple stmt)
{
- basic_block bb, rhs_bb;
+ basic_block bb, rhs_bb, lhs_bb;
edge e;
tree lhs, rhs;
use_operand_p use_p;
gimple use_stmt;
- if (!gimple_assign_cast_p (stmt))
+ if (!gimple_assign_cast_p (stmt)
+ && (!is_gimple_assign (stmt)
+ || (TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
+ != tcc_comparison)))
return false;
bb = gimple_bb (stmt);
if (!single_succ_p (bb))
@@ -2729,11 +2743,16 @@ final_range_test_p (gimple stmt)
lhs = gimple_assign_lhs (stmt);
rhs = gimple_assign_rhs1 (stmt);
- if (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
- || TREE_CODE (rhs) != SSA_NAME
- || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE)
+ if (gimple_assign_cast_p (stmt)
+ && (!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
+ || TREE_CODE (rhs) != SSA_NAME
+ || TREE_CODE (TREE_TYPE (rhs)) != BOOLEAN_TYPE))
return false;
+ if (!gimple_assign_cast_p (stmt)
+ && (TREE_CODE (TREE_TYPE (lhs)) != BOOLEAN_TYPE))
+ return false;
+
/* Test whether lhs is consumed only by a PHI in the only successor bb. */
if (!single_imm_use (lhs, &use_p, &use_stmt))
return false;
@@ -2743,10 +2762,20 @@ final_range_test_p (gimple stmt)
return false;
/* And that the rhs is defined in the same loop. */
- rhs_bb = gimple_bb (SSA_NAME_DEF_STMT (rhs));
- if (rhs_bb == NULL
- || !flow_bb_inside_loop_p (loop_containing_stmt (stmt), rhs_bb))
- return false;
+ if (gimple_assign_cast_p (stmt))
+ {
+ if (TREE_CODE (rhs) != SSA_NAME
+ || !(rhs_bb = gimple_bb (SSA_NAME_DEF_STMT (rhs)))
+ || !flow_bb_inside_loop_p (loop_containing_stmt (stmt), rhs_bb))
+ return false;
+ }
+ else
+ {
+ if (TREE_CODE (lhs) != SSA_NAME
+ || !(lhs_bb = gimple_bb (SSA_NAME_DEF_STMT (lhs)))
+ || !flow_bb_inside_loop_p (loop_containing_stmt (stmt), lhs_bb))
+ return false;
+ }
return true;
}
@@ -3132,6 +3161,8 @@ maybe_optimize_range_tests (gimple stmt)
/* stmt is
_123 = (int) _234;
+ OR
+ _234 = a_2(D) == 2;
followed by:
<bb M>:
@@ -3161,6 +3192,8 @@ maybe_optimize_range_tests (gimple stmt)
of the bitwise or resp. and, recursively. */
if (!get_ops (rhs, code, &ops,
loop_containing_stmt (stmt))
+ && (TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
+ != tcc_comparison)
&& has_single_use (rhs))
{
/* Otherwise, push the _234 range test itself. */
@@ -3173,6 +3206,23 @@ maybe_optimize_range_tests (gimple stmt)
ops.safe_push (oe);
bb_ent.last_idx++;
}
+ else if (!get_ops (lhs, code, &ops,
+ loop_containing_stmt (stmt))
+ && TREE_CODE (lhs) == SSA_NAME
+ && INTEGRAL_TYPE_P (TREE_TYPE (lhs))
+ && is_gimple_assign (stmt)
+ && (TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
+ == tcc_comparison)
+ && has_single_use (lhs))
+ {
+ operand_entry_t oe = operand_entry_pool.allocate ();
+ oe->op = lhs;
+ oe->rank = code;
+ oe->id = 0;
+ oe->count = 1;
+ ops.safe_push (oe);
+ bb_ent.last_idx++;
+ }
else
bb_ent.last_idx = ops.length ();
bb_ent.op = rhs;
@@ -3258,16 +3308,46 @@ maybe_optimize_range_tests (gimple stmt)
gimple use_stmt, cast_stmt = NULL;
FOR_EACH_IMM_USE_STMT (use_stmt, iter, bbinfo[idx].op)
- if (is_gimple_debug (use_stmt))
+ if (is_gimple_debug (use_stmt)
+ || (TREE_CODE (new_op) == SSA_NAME
+ && !reassoc_stmt_dominates_stmt_p
+ (SSA_NAME_DEF_STMT (new_op), use_stmt)))
continue;
- else if (gimple_code (use_stmt) == GIMPLE_COND
- || gimple_code (use_stmt) == GIMPLE_PHI)
- FOR_EACH_IMM_USE_ON_STMT (use_p, iter)
- SET_USE (use_p, new_op);
+ else if (gimple_code (use_stmt) == GIMPLE_PHI)
+ FOR_EACH_IMM_USE_ON_STMT (use_p, iter)
+ SET_USE (use_p, new_op);
+ else if (gimple_code (use_stmt) == GIMPLE_COND)
+ {
+ tree new_type, new_lhs;
+ gassign *g;
+ gcond *cond_stmt = as_a <gcond *> (use_stmt);
+ new_type = TREE_TYPE (gimple_cond_lhs (cond_stmt));
+ if (!types_compatible_p (new_type, TREE_TYPE (new_op)))
+ {
+ new_lhs = make_ssa_name (new_type);
+ if (is_gimple_min_invariant (new_op))
+ {
+ new_op = fold_convert (new_type, new_op);
+ g = gimple_build_assign (new_lhs, new_op);
+ }
+ else
+ g = gimple_build_assign (new_lhs,
+ CONVERT_EXPR, new_op);
+ gimple_stmt_iterator gsi = gsi_for_stmt (use_stmt);
+ gimple_set_uid (g, gimple_uid (use_stmt));
+ gimple_set_visited (g, true);
+ gsi_insert_before (&gsi, g, GSI_SAME_STMT);
+ }
+ else
+ new_lhs = new_op;
+ FOR_EACH_IMM_USE_ON_STMT (use_p, iter)
+ SET_USE (use_p, new_lhs);
+ }
else if (gimple_assign_cast_p (use_stmt))
cast_stmt = use_stmt;
else
- gcc_unreachable ();
+ cast_stmt = NULL;
+
if (cast_stmt)
{
gcc_assert (bb == last_bb);
@@ -3291,7 +3371,8 @@ maybe_optimize_range_tests (gimple stmt)
if (is_gimple_debug (use_stmt))
continue;
else if (gimple_code (use_stmt) == GIMPLE_COND
- || gimple_code (use_stmt) == GIMPLE_PHI)
+ || gimple_code (use_stmt) == GIMPLE_PHI
+ || is_gimple_assign (use_stmt))
FOR_EACH_IMM_USE_ON_STMT (use_p, iter)
SET_USE (use_p, new_lhs);
else
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PR66726] Factor conversion out of COND_EXPR
2015-07-27 3:10 ` Kugan
@ 2015-08-03 16:59 ` Jeff Law
0 siblings, 0 replies; 16+ messages in thread
From: Jeff Law @ 2015-08-03 16:59 UTC (permalink / raw)
To: Kugan, Bernhard Reutner-Fischer; +Cc: gcc-patches
On 07/26/2015 07:05 PM, Kugan wrote:
>
> I thought that when !gimple_assign_cast_p (stmt), RHS will always
> boolean. I have now added this check in the attached patch.
Thanks.
>
> I also noticed that in maybe_optimize_range_tests, GIMPLE_COND can
> have non compatible types when new_op is updated
> (boolean types coming from tcc_compare results) and hence need to be
> converted. Changed that as well.
Did you find this by examination or with some testcode? If the latter,
including a testcase for this issue would be appreciated.
>
> Bootstrapped and regression tested on x86-64-none-linux-gnu with no new
> regressions. Is this OK for trunk?
OK with an updated changelog.
Jeff
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2015-08-03 16:59 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-04 2:59 [PR66726] Factor conversion out of COND_EXPR Kugan
2015-07-04 8:51 ` Bernhard Reutner-Fischer
2015-07-04 12:32 ` Kugan
2015-07-04 13:21 ` Bernhard Reutner-Fischer
2015-07-06 21:37 ` Jeff Law
2015-07-07 12:50 ` Kugan
2015-07-07 14:40 ` Jeff Law
2015-07-09 23:08 ` Kugan
2015-07-10 20:40 ` Jeff Law
2015-07-12 11:24 ` Kugan
2015-07-15 7:49 ` Kugan
2015-07-15 18:55 ` Jeff Law
2015-07-16 7:06 ` Kugan
2015-07-23 19:48 ` Jeff Law
2015-07-27 3:10 ` Kugan
2015-08-03 16:59 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).