From: Richard Biener <richard.guenther@gmail.com>
To: Marek Polacek <polacek@redhat.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>, Jeff Law <law@redhat.com>,
Kugan <kugan.vivekanandarajah@linaro.org>
Subject: Re: [PATCH] Fix phiopt ICE in Factor conversion in COND_EXPR (PR tree-optimization/66949)
Date: Wed, 09 Dec 2015 10:41:00 -0000 [thread overview]
Message-ID: <CAFiYyc1B+AGP-R38wkE=kqmdenAMZar+kp=77VEtOTX+dxkMhw@mail.gmail.com> (raw)
In-Reply-To: <20151208162133.GQ3175@redhat.com>
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
next prev parent reply other threads:[~2015-12-09 10:41 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-08 16:21 Marek Polacek
2015-12-08 23:15 ` Kugan
2015-12-09 10:41 ` Richard Biener [this message]
2015-12-09 14:22 ` Marek Polacek
2015-12-09 14:37 ` Richard Biener
2015-12-10 22:27 ` Jeff Law
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFiYyc1B+AGP-R38wkE=kqmdenAMZar+kp=77VEtOTX+dxkMhw@mail.gmail.com' \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=kugan.vivekanandarajah@linaro.org \
--cc=law@redhat.com \
--cc=polacek@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).