* [PATCH] Fix phiopt ICE in Factor conversion in COND_EXPR (PR tree-optimization/66949)
@ 2015-12-08 16:21 Marek Polacek
2015-12-08 23:15 ` Kugan
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Marek Polacek @ 2015-12-08 16:21 UTC (permalink / raw)
To: GCC Patches, Jeff Law; +Cc: Kugan
The following is a conservative fix for this PR. This is an ICE transpiring
in the new "Factor conversion in COND_EXPR" optimization added in r225722.
Before this optimization kicks in, we have
<bb 2>:
...
p1_32 = (short unsigned int) _20;
<bb 3>:
...
iftmp.0_18 = (short unsigned int) _20;
<bb 4>:
...
# iftmp.0_19 = PHI <iftmp.0_18(3), p1_32(2)>
after factor_out_conditional_conversion does its work, we end up with those two
def stmts removed and instead of the PHI we'll have
# _35 = PHI <_20(3), _20(2)>
iftmp.0_19 = (short unsigned int) _35;
That itself looks like a fine optimization, but after factor_out_conditional_conversion
there's
320 phis = phi_nodes (bb2);
321 phi = single_non_singleton_phi_for_edges (phis, e1, e2);
322 gcc_assert (phi);
and phis look like
b.2_38 = PHI <b.2_9(3), b.2_9(2)>
_35 = PHI <_20(3), _20(2)>
so single_non_singleton_phi_for_edges returns NULL and the subsequent assert triggers.
With this patch we won't ICE (and PRE should clean this up anyway), but I don't know,
maybe I should try harder to optimize even this problematical case (not sure how hard
it would be...)?
pr66949-2.c only ICEd on powerpc64le and I have verified that this patch fixes it too.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2015-12-08 Marek Polacek <polacek@redhat.com>
PR tree-optimization/66949
* tree-ssa-phiopt.c (factor_out_conditional_conversion): Return false if
NEW_ARG0 and NEW_ARG1 are equal.
* gcc.dg/torture/pr66949-1.c: New test.
* gcc.dg/torture/pr66949-2.c: New test.
diff --git gcc/testsuite/gcc.dg/torture/pr66949-1.c gcc/testsuite/gcc.dg/torture/pr66949-1.c
index e69de29..1b765bc 100644
--- gcc/testsuite/gcc.dg/torture/pr66949-1.c
+++ gcc/testsuite/gcc.dg/torture/pr66949-1.c
@@ -0,0 +1,28 @@
+/* PR tree-optimization/66949 */
+/* { dg-do compile } */
+
+int a, *b = &a, c;
+
+unsigned short
+fn1 (unsigned short p1, unsigned int p2)
+{
+ return p2 > 1 || p1 >> p2 ? p1 : p1 << p2;
+}
+
+void
+fn2 ()
+{
+ int *d = &a;
+ for (a = 0; a < -1; a = 1)
+ ;
+ if (a < 0)
+ c = 0;
+ *b = fn1 (*d || c, *d);
+}
+
+int
+main ()
+{
+ fn2 ();
+ return 0;
+}
diff --git gcc/testsuite/gcc.dg/torture/pr66949-2.c gcc/testsuite/gcc.dg/torture/pr66949-2.c
index e69de29..e6250a3 100644
--- gcc/testsuite/gcc.dg/torture/pr66949-2.c
+++ gcc/testsuite/gcc.dg/torture/pr66949-2.c
@@ -0,0 +1,23 @@
+/* PR tree-optimization/66949 */
+/* { dg-do compile } */
+
+char a;
+int b, c, d;
+extern int fn2 (void);
+
+short
+fn1 (short p1, short p2)
+{
+ return p2 == 0 ? p1 : p1 / p2;
+}
+
+int
+main (void)
+{
+ char e = 1;
+ int f = 7;
+ c = a >> f;
+ b = fn1 (c, 0 < d <= e && fn2 ());
+
+ return 0;
+}
diff --git gcc/tree-ssa-phiopt.c gcc/tree-ssa-phiopt.c
index 344cd2f..caac5d5 100644
--- gcc/tree-ssa-phiopt.c
+++ gcc/tree-ssa-phiopt.c
@@ -477,6 +477,11 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
return false;
}
+ /* If we were to continue, we'd create a PHI with same arguments for edges
+ E0 and E1. That could get us in trouble later, so punt. */
+ if (operand_equal_for_phi_arg_p (new_arg0, new_arg1))
+ 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)
Marek
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix phiopt ICE in Factor conversion in COND_EXPR (PR tree-optimization/66949)
2015-12-08 16:21 [PATCH] Fix phiopt ICE in Factor conversion in COND_EXPR (PR tree-optimization/66949) Marek Polacek
@ 2015-12-08 23:15 ` Kugan
2015-12-09 10:41 ` Richard Biener
2015-12-10 22:27 ` Jeff Law
2 siblings, 0 replies; 6+ messages in thread
From: Kugan @ 2015-12-08 23:15 UTC (permalink / raw)
To: Marek Polacek, GCC Patches, Jeff Law
On 09/12/15 03:21, Marek Polacek wrote:
> The following is a conservative fix for this PR. This is an ICE transpiring
> in the new "Factor conversion in COND_EXPR" optimization added in r225722.
>
> Before this optimization kicks in, we have
> <bb 2>:
> ...
> p1_32 = (short unsigned int) _20;
>
> <bb 3>:
> ...
> iftmp.0_18 = (short unsigned int) _20;
>
> <bb 4>:
> ...
> # iftmp.0_19 = PHI <iftmp.0_18(3), p1_32(2)>
>
> after factor_out_conditional_conversion does its work, we end up with those two
> def stmts removed and instead of the PHI we'll have
>
> # _35 = PHI <_20(3), _20(2)>
> iftmp.0_19 = (short unsigned int) _35;
>
> That itself looks like a fine optimization, but after factor_out_conditional_conversion
> there's
> 320 phis = phi_nodes (bb2);
> 321 phi = single_non_singleton_phi_for_edges (phis, e1, e2);
> 322 gcc_assert (phi);
> and phis look like
> b.2_38 = PHI <b.2_9(3), b.2_9(2)>
> _35 = PHI <_20(3), _20(2)>
> so single_non_singleton_phi_for_edges returns NULL and the subsequent assert triggers.
>
> With this patch we won't ICE (and PRE should clean this up anyway), but I don't know,
> maybe I should try harder to optimize even this problematical case (not sure how hard
> it would be...)?
Hi Marek,
Thanks for fixing this. Yes, we can try remove the PHI in
factor_out_conditional_conversion. But as you said, it might not be
important. In any case, please let me know if you want me to try doing that.
Thanks,
Kugan
>
> pr66949-2.c only ICEd on powerpc64le and I have verified that this patch fixes it too.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2015-12-08 Marek Polacek <polacek@redhat.com>
>
> PR tree-optimization/66949
> * tree-ssa-phiopt.c (factor_out_conditional_conversion): Return false if
> NEW_ARG0 and NEW_ARG1 are equal.
>
> * gcc.dg/torture/pr66949-1.c: New test.
> * gcc.dg/torture/pr66949-2.c: New test.
>
> diff --git gcc/testsuite/gcc.dg/torture/pr66949-1.c gcc/testsuite/gcc.dg/torture/pr66949-1.c
> index e69de29..1b765bc 100644
> --- gcc/testsuite/gcc.dg/torture/pr66949-1.c
> +++ gcc/testsuite/gcc.dg/torture/pr66949-1.c
> @@ -0,0 +1,28 @@
> +/* PR tree-optimization/66949 */
> +/* { dg-do compile } */
> +
> +int a, *b = &a, c;
> +
> +unsigned short
> +fn1 (unsigned short p1, unsigned int p2)
> +{
> + return p2 > 1 || p1 >> p2 ? p1 : p1 << p2;
> +}
> +
> +void
> +fn2 ()
> +{
> + int *d = &a;
> + for (a = 0; a < -1; a = 1)
> + ;
> + if (a < 0)
> + c = 0;
> + *b = fn1 (*d || c, *d);
> +}
> +
> +int
> +main ()
> +{
> + fn2 ();
> + return 0;
> +}
> diff --git gcc/testsuite/gcc.dg/torture/pr66949-2.c gcc/testsuite/gcc.dg/torture/pr66949-2.c
> index e69de29..e6250a3 100644
> --- gcc/testsuite/gcc.dg/torture/pr66949-2.c
> +++ gcc/testsuite/gcc.dg/torture/pr66949-2.c
> @@ -0,0 +1,23 @@
> +/* PR tree-optimization/66949 */
> +/* { dg-do compile } */
> +
> +char a;
> +int b, c, d;
> +extern int fn2 (void);
> +
> +short
> +fn1 (short p1, short p2)
> +{
> + return p2 == 0 ? p1 : p1 / p2;
> +}
> +
> +int
> +main (void)
> +{
> + char e = 1;
> + int f = 7;
> + c = a >> f;
> + b = fn1 (c, 0 < d <= e && fn2 ());
> +
> + return 0;
> +}
> diff --git gcc/tree-ssa-phiopt.c gcc/tree-ssa-phiopt.c
> index 344cd2f..caac5d5 100644
> --- gcc/tree-ssa-phiopt.c
> +++ gcc/tree-ssa-phiopt.c
> @@ -477,6 +477,11 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
> return false;
> }
>
> + /* If we were to continue, we'd create a PHI with same arguments for edges
> + E0 and E1. That could get us in trouble later, so punt. */
> + if (operand_equal_for_phi_arg_p (new_arg0, new_arg1))
> + 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)
>
> Marek
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix phiopt ICE in Factor conversion in COND_EXPR (PR tree-optimization/66949)
2015-12-08 16:21 [PATCH] Fix phiopt ICE in Factor conversion in COND_EXPR (PR tree-optimization/66949) Marek Polacek
2015-12-08 23:15 ` Kugan
@ 2015-12-09 10:41 ` Richard Biener
2015-12-09 14:22 ` Marek Polacek
2015-12-10 22:27 ` Jeff Law
2 siblings, 1 reply; 6+ messages in thread
From: Richard Biener @ 2015-12-09 10:41 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches, Jeff Law, Kugan
On Tue, Dec 8, 2015 at 5:21 PM, Marek Polacek <polacek@redhat.com> wrote:
> The following is a conservative fix for this PR. This is an ICE transpiring
> in the new "Factor conversion in COND_EXPR" optimization added in r225722.
>
> Before this optimization kicks in, we have
> <bb 2>:
> ...
> p1_32 = (short unsigned int) _20;
>
> <bb 3>:
> ...
> iftmp.0_18 = (short unsigned int) _20;
>
> <bb 4>:
> ...
> # iftmp.0_19 = PHI <iftmp.0_18(3), p1_32(2)>
>
> after factor_out_conditional_conversion does its work, we end up with those two
> def stmts removed and instead of the PHI we'll have
>
> # _35 = PHI <_20(3), _20(2)>
> iftmp.0_19 = (short unsigned int) _35;
>
> That itself looks like a fine optimization, but after factor_out_conditional_conversion
> there's
> 320 phis = phi_nodes (bb2);
> 321 phi = single_non_singleton_phi_for_edges (phis, e1, e2);
> 322 gcc_assert (phi);
> and phis look like
> b.2_38 = PHI <b.2_9(3), b.2_9(2)>
> _35 = PHI <_20(3), _20(2)>
> so single_non_singleton_phi_for_edges returns NULL and the subsequent assert triggers.
>
> With this patch we won't ICE (and PRE should clean this up anyway), but I don't know,
> maybe I should try harder to optimize even this problematical case (not sure how hard
> it would be...)?
>
> pr66949-2.c only ICEd on powerpc64le and I have verified that this patch fixes it too.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
Hmm.
Yes, there is an opportunity for optimization.
But I don't see why we have the asserts at all - they seem to be originated from
development. IMHO factor_out_conditonal_conversion should simply return
the new PHI it generates instead of relying on
signle_non_signelton_phi_for_edges
to return it.
Richard.
> 2015-12-08 Marek Polacek <polacek@redhat.com>
>
> PR tree-optimization/66949
> * tree-ssa-phiopt.c (factor_out_conditional_conversion): Return false if
> NEW_ARG0 and NEW_ARG1 are equal.
>
> * gcc.dg/torture/pr66949-1.c: New test.
> * gcc.dg/torture/pr66949-2.c: New test.
>
> diff --git gcc/testsuite/gcc.dg/torture/pr66949-1.c gcc/testsuite/gcc.dg/torture/pr66949-1.c
> index e69de29..1b765bc 100644
> --- gcc/testsuite/gcc.dg/torture/pr66949-1.c
> +++ gcc/testsuite/gcc.dg/torture/pr66949-1.c
> @@ -0,0 +1,28 @@
> +/* PR tree-optimization/66949 */
> +/* { dg-do compile } */
> +
> +int a, *b = &a, c;
> +
> +unsigned short
> +fn1 (unsigned short p1, unsigned int p2)
> +{
> + return p2 > 1 || p1 >> p2 ? p1 : p1 << p2;
> +}
> +
> +void
> +fn2 ()
> +{
> + int *d = &a;
> + for (a = 0; a < -1; a = 1)
> + ;
> + if (a < 0)
> + c = 0;
> + *b = fn1 (*d || c, *d);
> +}
> +
> +int
> +main ()
> +{
> + fn2 ();
> + return 0;
> +}
> diff --git gcc/testsuite/gcc.dg/torture/pr66949-2.c gcc/testsuite/gcc.dg/torture/pr66949-2.c
> index e69de29..e6250a3 100644
> --- gcc/testsuite/gcc.dg/torture/pr66949-2.c
> +++ gcc/testsuite/gcc.dg/torture/pr66949-2.c
> @@ -0,0 +1,23 @@
> +/* PR tree-optimization/66949 */
> +/* { dg-do compile } */
> +
> +char a;
> +int b, c, d;
> +extern int fn2 (void);
> +
> +short
> +fn1 (short p1, short p2)
> +{
> + return p2 == 0 ? p1 : p1 / p2;
> +}
> +
> +int
> +main (void)
> +{
> + char e = 1;
> + int f = 7;
> + c = a >> f;
> + b = fn1 (c, 0 < d <= e && fn2 ());
> +
> + return 0;
> +}
> diff --git gcc/tree-ssa-phiopt.c gcc/tree-ssa-phiopt.c
> index 344cd2f..caac5d5 100644
> --- gcc/tree-ssa-phiopt.c
> +++ gcc/tree-ssa-phiopt.c
> @@ -477,6 +477,11 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
> return false;
> }
>
> + /* If we were to continue, we'd create a PHI with same arguments for edges
> + E0 and E1. That could get us in trouble later, so punt. */
> + if (operand_equal_for_phi_arg_p (new_arg0, new_arg1))
> + 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)
>
> Marek
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix phiopt ICE in Factor conversion in COND_EXPR (PR tree-optimization/66949)
2015-12-09 10:41 ` Richard Biener
@ 2015-12-09 14:22 ` Marek Polacek
2015-12-09 14:37 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Marek Polacek @ 2015-12-09 14:22 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches, Jeff Law, Kugan
On Wed, Dec 09, 2015 at 11:41:44AM +0100, Richard Biener wrote:
> But I don't see why we have the asserts at all - they seem to be originated from
> development. IMHO factor_out_conditonal_conversion should simply return
> the new PHI it generates instead of relying on
> signle_non_signelton_phi_for_edges
> to return it.
Ok, that seems to work.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2015-12-09 Marek Polacek <polacek@redhat.com>
PR tree-optimization/66949
* tree-ssa-phiopt.c (tree_ssa_phiopt_worker): Don't call
single_non_singleton_phi_for_edges to get the PHI from
factor_out_conditional_conversion. Use NULL_TREE instead of NULL.
(factor_out_conditional_conversion): Adjust declaration. Make it
return the newly-created PHI.
* gcc.dg/torture/pr66949-1.c: New test.
* gcc.dg/torture/pr66949-2.c: New test.
diff --git gcc/testsuite/gcc.dg/torture/pr66949-1.c gcc/testsuite/gcc.dg/torture/pr66949-1.c
index e69de29..1b765bc 100644
--- gcc/testsuite/gcc.dg/torture/pr66949-1.c
+++ gcc/testsuite/gcc.dg/torture/pr66949-1.c
@@ -0,0 +1,28 @@
+/* PR tree-optimization/66949 */
+/* { dg-do compile } */
+
+int a, *b = &a, c;
+
+unsigned short
+fn1 (unsigned short p1, unsigned int p2)
+{
+ return p2 > 1 || p1 >> p2 ? p1 : p1 << p2;
+}
+
+void
+fn2 ()
+{
+ int *d = &a;
+ for (a = 0; a < -1; a = 1)
+ ;
+ if (a < 0)
+ c = 0;
+ *b = fn1 (*d || c, *d);
+}
+
+int
+main ()
+{
+ fn2 ();
+ return 0;
+}
diff --git gcc/testsuite/gcc.dg/torture/pr66949-2.c gcc/testsuite/gcc.dg/torture/pr66949-2.c
index e69de29..e6250a3 100644
--- gcc/testsuite/gcc.dg/torture/pr66949-2.c
+++ gcc/testsuite/gcc.dg/torture/pr66949-2.c
@@ -0,0 +1,23 @@
+/* PR tree-optimization/66949 */
+/* { dg-do compile } */
+
+char a;
+int b, c, d;
+extern int fn2 (void);
+
+short
+fn1 (short p1, short p2)
+{
+ return p2 == 0 ? p1 : p1 / p2;
+}
+
+int
+main (void)
+{
+ char e = 1;
+ int f = 7;
+ c = a >> f;
+ b = fn1 (c, 0 < d <= e && fn2 ());
+
+ return 0;
+}
diff --git gcc/tree-ssa-phiopt.c gcc/tree-ssa-phiopt.c
index 344cd2f..d7e8aa0 100644
--- gcc/tree-ssa-phiopt.c
+++ gcc/tree-ssa-phiopt.c
@@ -49,7 +49,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 gphi *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,
@@ -310,19 +310,19 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads)
/* Something is wrong if we cannot find the arguments in the PHI
node. */
- gcc_assert (arg0 != NULL && arg1 != NULL);
+ gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
- if (factor_out_conditional_conversion (e1, e2, phi, arg0, arg1))
+ gphi *newphi = factor_out_conditional_conversion (e1, e2, phi,
+ arg0, arg1);
+ if (newphi != NULL)
{
+ phi = newphi;
/* 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);
+ gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
}
/* Do the replacement of conditional if it can be done. */
@@ -402,9 +402,9 @@ replace_phi_edge_with_variable (basic_block cond_block,
/* 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. */
+ to the result of PHI stmt. Return the newly-created PHI, if any. */
-static bool
+static gphi *
factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
tree arg0, tree arg1)
{
@@ -421,7 +421,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
statement have the same unary operation, we can handle more
than two arguments too. */
if (gimple_phi_num_args (phi) != 2)
- return false;
+ return NULL;
/* First canonicalize to simplify tests. */
if (TREE_CODE (arg0) != SSA_NAME)
@@ -433,14 +433,14 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
if (TREE_CODE (arg0) != SSA_NAME
|| (TREE_CODE (arg1) != SSA_NAME
&& TREE_CODE (arg1) != INTEGER_CST))
- return false;
+ return NULL;
/* 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;
+ return NULL;
/* Use the RHS as new_arg0. */
convert_code = gimple_assign_rhs_code (arg0_def_stmt);
@@ -455,7 +455,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
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;
+ return NULL;
/* Use the RHS as new_arg1. */
new_arg1 = gimple_assign_rhs1 (arg1_def_stmt);
@@ -471,21 +471,21 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
if (gimple_assign_cast_p (arg0_def_stmt))
new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);
else
- return false;
+ return NULL;
}
else
- return false;
+ return NULL;
}
/* 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;
+ return NULL;
/* If types of new_arg0 and new_arg1 are different bailout. */
if (!types_compatible_p (TREE_TYPE (new_arg0), TREE_TYPE (new_arg1)))
- return false;
+ return NULL;
/* Create a new PHI stmt. */
result = PHI_RESULT (phi);
@@ -528,7 +528,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
/* Remove the original PHI stmt. */
gsi = gsi_for_stmt (phi);
gsi_remove (&gsi, true);
- return true;
+ return newphi;
}
/* The function conditional_replacement does the main work of doing the
Marek
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix phiopt ICE in Factor conversion in COND_EXPR (PR tree-optimization/66949)
2015-12-09 14:22 ` Marek Polacek
@ 2015-12-09 14:37 ` Richard Biener
0 siblings, 0 replies; 6+ messages in thread
From: Richard Biener @ 2015-12-09 14:37 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches, Jeff Law, Kugan
On Wed, Dec 9, 2015 at 3:22 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Wed, Dec 09, 2015 at 11:41:44AM +0100, Richard Biener wrote:
>> But I don't see why we have the asserts at all - they seem to be originated from
>> development. IMHO factor_out_conditonal_conversion should simply return
>> the new PHI it generates instead of relying on
>> signle_non_signelton_phi_for_edges
>> to return it.
>
> Ok, that seems to work.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
Ok.
Thanks,
Richard.
> 2015-12-09 Marek Polacek <polacek@redhat.com>
>
> PR tree-optimization/66949
> * tree-ssa-phiopt.c (tree_ssa_phiopt_worker): Don't call
> single_non_singleton_phi_for_edges to get the PHI from
> factor_out_conditional_conversion. Use NULL_TREE instead of NULL.
> (factor_out_conditional_conversion): Adjust declaration. Make it
> return the newly-created PHI.
>
> * gcc.dg/torture/pr66949-1.c: New test.
> * gcc.dg/torture/pr66949-2.c: New test.
>
> diff --git gcc/testsuite/gcc.dg/torture/pr66949-1.c gcc/testsuite/gcc.dg/torture/pr66949-1.c
> index e69de29..1b765bc 100644
> --- gcc/testsuite/gcc.dg/torture/pr66949-1.c
> +++ gcc/testsuite/gcc.dg/torture/pr66949-1.c
> @@ -0,0 +1,28 @@
> +/* PR tree-optimization/66949 */
> +/* { dg-do compile } */
> +
> +int a, *b = &a, c;
> +
> +unsigned short
> +fn1 (unsigned short p1, unsigned int p2)
> +{
> + return p2 > 1 || p1 >> p2 ? p1 : p1 << p2;
> +}
> +
> +void
> +fn2 ()
> +{
> + int *d = &a;
> + for (a = 0; a < -1; a = 1)
> + ;
> + if (a < 0)
> + c = 0;
> + *b = fn1 (*d || c, *d);
> +}
> +
> +int
> +main ()
> +{
> + fn2 ();
> + return 0;
> +}
> diff --git gcc/testsuite/gcc.dg/torture/pr66949-2.c gcc/testsuite/gcc.dg/torture/pr66949-2.c
> index e69de29..e6250a3 100644
> --- gcc/testsuite/gcc.dg/torture/pr66949-2.c
> +++ gcc/testsuite/gcc.dg/torture/pr66949-2.c
> @@ -0,0 +1,23 @@
> +/* PR tree-optimization/66949 */
> +/* { dg-do compile } */
> +
> +char a;
> +int b, c, d;
> +extern int fn2 (void);
> +
> +short
> +fn1 (short p1, short p2)
> +{
> + return p2 == 0 ? p1 : p1 / p2;
> +}
> +
> +int
> +main (void)
> +{
> + char e = 1;
> + int f = 7;
> + c = a >> f;
> + b = fn1 (c, 0 < d <= e && fn2 ());
> +
> + return 0;
> +}
> diff --git gcc/tree-ssa-phiopt.c gcc/tree-ssa-phiopt.c
> index 344cd2f..d7e8aa0 100644
> --- gcc/tree-ssa-phiopt.c
> +++ gcc/tree-ssa-phiopt.c
> @@ -49,7 +49,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 gphi *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,
> @@ -310,19 +310,19 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads)
>
> /* Something is wrong if we cannot find the arguments in the PHI
> node. */
> - gcc_assert (arg0 != NULL && arg1 != NULL);
> + gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
>
> - if (factor_out_conditional_conversion (e1, e2, phi, arg0, arg1))
> + gphi *newphi = factor_out_conditional_conversion (e1, e2, phi,
> + arg0, arg1);
> + if (newphi != NULL)
> {
> + phi = newphi;
> /* 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);
> + gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);
> }
>
> /* Do the replacement of conditional if it can be done. */
> @@ -402,9 +402,9 @@ replace_phi_edge_with_variable (basic_block cond_block,
>
> /* 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. */
> + to the result of PHI stmt. Return the newly-created PHI, if any. */
>
> -static bool
> +static gphi *
> factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
> tree arg0, tree arg1)
> {
> @@ -421,7 +421,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
> statement have the same unary operation, we can handle more
> than two arguments too. */
> if (gimple_phi_num_args (phi) != 2)
> - return false;
> + return NULL;
>
> /* First canonicalize to simplify tests. */
> if (TREE_CODE (arg0) != SSA_NAME)
> @@ -433,14 +433,14 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
> if (TREE_CODE (arg0) != SSA_NAME
> || (TREE_CODE (arg1) != SSA_NAME
> && TREE_CODE (arg1) != INTEGER_CST))
> - return false;
> + return NULL;
>
> /* 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;
> + return NULL;
>
> /* Use the RHS as new_arg0. */
> convert_code = gimple_assign_rhs_code (arg0_def_stmt);
> @@ -455,7 +455,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
> 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;
> + return NULL;
>
> /* Use the RHS as new_arg1. */
> new_arg1 = gimple_assign_rhs1 (arg1_def_stmt);
> @@ -471,21 +471,21 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
> if (gimple_assign_cast_p (arg0_def_stmt))
> new_arg1 = fold_convert (TREE_TYPE (new_arg0), arg1);
> else
> - return false;
> + return NULL;
> }
> else
> - return false;
> + return NULL;
> }
>
> /* 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;
> + return NULL;
>
> /* If types of new_arg0 and new_arg1 are different bailout. */
> if (!types_compatible_p (TREE_TYPE (new_arg0), TREE_TYPE (new_arg1)))
> - return false;
> + return NULL;
>
> /* Create a new PHI stmt. */
> result = PHI_RESULT (phi);
> @@ -528,7 +528,7 @@ factor_out_conditional_conversion (edge e0, edge e1, gphi *phi,
> /* Remove the original PHI stmt. */
> gsi = gsi_for_stmt (phi);
> gsi_remove (&gsi, true);
> - return true;
> + return newphi;
> }
>
> /* The function conditional_replacement does the main work of doing the
>
> Marek
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix phiopt ICE in Factor conversion in COND_EXPR (PR tree-optimization/66949)
2015-12-08 16:21 [PATCH] Fix phiopt ICE in Factor conversion in COND_EXPR (PR tree-optimization/66949) Marek Polacek
2015-12-08 23:15 ` Kugan
2015-12-09 10:41 ` Richard Biener
@ 2015-12-10 22:27 ` Jeff Law
2 siblings, 0 replies; 6+ messages in thread
From: Jeff Law @ 2015-12-10 22:27 UTC (permalink / raw)
To: Marek Polacek, GCC Patches; +Cc: Kugan
On 12/08/2015 09:21 AM, Marek Polacek wrote:
> The following is a conservative fix for this PR. This is an ICE transpiring
> in the new "Factor conversion in COND_EXPR" optimization added in r225722.
>
> Before this optimization kicks in, we have
> <bb 2>:
> ...
> p1_32 = (short unsigned int) _20;
>
> <bb 3>:
> ...
> iftmp.0_18 = (short unsigned int) _20;
>
> <bb 4>:
> ...
> # iftmp.0_19 = PHI <iftmp.0_18(3), p1_32(2)>
>
> after factor_out_conditional_conversion does its work, we end up with those two
> def stmts removed and instead of the PHI we'll have
>
> # _35 = PHI <_20(3), _20(2)>
> iftmp.0_19 = (short unsigned int) _35;
>
> That itself looks like a fine optimization, but after factor_out_conditional_conversion
> there's
> 320 phis = phi_nodes (bb2);
> 321 phi = single_non_singleton_phi_for_edges (phis, e1, e2);
> 322 gcc_assert (phi);
> and phis look like
> b.2_38 = PHI <b.2_9(3), b.2_9(2)>
> _35 = PHI <_20(3), _20(2)>
> so single_non_singleton_phi_for_edges returns NULL and the subsequent assert triggers.
Cute.
>
> With this patch we won't ICE (and PRE should clean this up anyway), but I don't know,
> maybe I should try harder to optimize even this problematical case (not sure how hard
> it would be...)?
So if this kind of situation is created by the first phiopt, then DOM
should fix it. As you note PRE will catch it if the second phiopt pass
creates this degenerate PHI. If created by the last phiopt pass, then
hopefully coalescing would save us.
Jeff
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-12-10 22:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-08 16:21 [PATCH] Fix phiopt ICE in Factor conversion in COND_EXPR (PR tree-optimization/66949) Marek Polacek
2015-12-08 23:15 ` Kugan
2015-12-09 10:41 ` Richard Biener
2015-12-09 14:22 ` Marek Polacek
2015-12-09 14:37 ` Richard Biener
2015-12-10 22:27 ` 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).