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 <pola...@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
>
