public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Marek Polacek <polacek@redhat.com>
To: GCC Patches <gcc-patches@gcc.gnu.org>, Jeff Law <law@redhat.com>
Cc: Kugan <kugan.vivekanandarajah@linaro.org>
Subject: [PATCH] Fix phiopt ICE in Factor conversion in COND_EXPR (PR tree-optimization/66949)
Date: Tue, 08 Dec 2015 16:21:00 -0000	[thread overview]
Message-ID: <20151208162133.GQ3175@redhat.com> (raw)

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

             reply	other threads:[~2015-12-08 16:21 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-12-08 16:21 Marek Polacek [this message]
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

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=20151208162133.GQ3175@redhat.com \
    --to=polacek@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=kugan.vivekanandarajah@linaro.org \
    --cc=law@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).