diff mbox

Fix ICE in predicate_mem_writes (PR tree-optimization/70725)

Message ID 20160419183549.GN28445@redhat.com
State New
Headers show

Commit Message

Marek Polacek April 19, 2016, 6:35 p.m. UTC 
While predicate_mem_writes has a check to skip conditions that were evaluated
to true, it's lacking the same check for false, so we hit an assert later on.
So I'm adding is_false_predicate.  Maybe it should be added to other spots as
well, but I'm not sure about that.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-04-19  Marek Polacek  <polacek@redhat.com>

	PR tree-optimization/70725
	* tree-if-conv.c (is_false_predicate): New function.
	(predicate_mem_writes): Use it.

	* gcc.dg/pr70725.c: New test.


	Marek

Comments

Richard Biener April 20, 2016, 9:04 a.m. UTC | #1 
On Tue, Apr 19, 2016 at 8:35 PM, Marek Polacek <polacek@redhat.com> wrote:
> While predicate_mem_writes has a check to skip conditions that were evaluated
> to true, it's lacking the same check for false, so we hit an assert later on.
> So I'm adding is_false_predicate.  Maybe it should be added to other spots as
> well, but I'm not sure about that.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?

Ok.

Thanks,
Richard.

> 2016-04-19  Marek Polacek  <polacek@redhat.com>
>
>         PR tree-optimization/70725
>         * tree-if-conv.c (is_false_predicate): New function.
>         (predicate_mem_writes): Use it.
>
>         * gcc.dg/pr70725.c: New test.
>
> diff --git gcc/testsuite/gcc.dg/pr70725.c gcc/testsuite/gcc.dg/pr70725.c
> index e69de29..fc7b674 100644
> --- gcc/testsuite/gcc.dg/pr70725.c
> +++ gcc/testsuite/gcc.dg/pr70725.c
> @@ -0,0 +1,22 @@
> +/* PR tree-optimization/70725 */
> +/* { dg-do compile } */
> +/* { dg-options "-O3" } */
> +/* { dg-additional-options "-march=skylake-avx512" { target { i?86-*-* x86_64-*-* } } } */
> +
> +extern short a;
> +extern int b, d;
> +extern int c[100];
> +extern int e;
> +extern int f;
> +
> +void
> +fn1 ()
> +{
> +  for (; e < 2; e = e + 1)
> +    d = a;
> +  for (;;)
> +    for (int g = 0; g < 5; g = g + 1)
> +      for (int h = 0; h < 2; h = h + 1)
> +       for (int i = 0; i < 3; i = i + 1)
> +         c[f + i] = a && b;
> +}
> diff --git gcc/tree-if-conv.c gcc/tree-if-conv.c
> index 9e305c7..a9fbab9 100644
> --- gcc/tree-if-conv.c
> +++ gcc/tree-if-conv.c
> @@ -262,6 +262,16 @@ ifc_temp_var (tree type, tree expr, gimple_stmt_iterator *gsi)
>    return new_name;
>  }
>
> +/* Return true when COND is a false predicate.  */
> +
> +static inline bool
> +is_false_predicate (tree cond)
> +{
> +  return (cond == NULL_TREE
> +         || cond == boolean_false_node
> +         || integer_zerop (cond));
> +}
> +
>  /* Return true when COND is a true predicate.  */
>
>  static inline bool
> @@ -1988,7 +1998,7 @@ predicate_mem_writes (loop_p loop)
>        gimple *stmt;
>        int index;
>
> -      if (is_true_predicate (cond))
> +      if (is_true_predicate (cond) || is_false_predicate (cond))
>         continue;
>
>        swap = false;
>
>         Marek
Marek Polacek April 20, 2016, 9:42 a.m. UTC | #2 
On Wed, Apr 20, 2016 at 11:04:08AM +0200, Richard Biener wrote:
> On Tue, Apr 19, 2016 at 8:35 PM, Marek Polacek <polacek@redhat.com> wrote:
> > While predicate_mem_writes has a check to skip conditions that were evaluated
> > to true, it's lacking the same check for false, so we hit an assert later on.
> > So I'm adding is_false_predicate.  Maybe it should be added to other spots as
> > well, but I'm not sure about that.
> >
> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> Ok.

Thanks, should I backport this to gcc-6-branch now?  Or wait until after 6.1?

	Marek
Richard Biener April 20, 2016, 9:47 a.m. UTC | #3 
On Wed, Apr 20, 2016 at 11:42 AM, Marek Polacek <polacek@redhat.com> wrote:
> On Wed, Apr 20, 2016 at 11:04:08AM +0200, Richard Biener wrote:
>> On Tue, Apr 19, 2016 at 8:35 PM, Marek Polacek <polacek@redhat.com> wrote:
>> > While predicate_mem_writes has a check to skip conditions that were evaluated
>> > to true, it's lacking the same check for false, so we hit an assert later on.
>> > So I'm adding is_false_predicate.  Maybe it should be added to other spots as
>> > well, but I'm not sure about that.
>> >
>> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
>>
>> Ok.
>
> Thanks, should I backport this to gcc-6-branch now?  Or wait until after 6.1?

It's fine to backport now as it's probably a regression.

Richard.

>         Marek
Marek Polacek April 20, 2016, 9:57 a.m. UTC | #4 
On Wed, Apr 20, 2016 at 11:47:07AM +0200, Richard Biener wrote:
> On Wed, Apr 20, 2016 at 11:42 AM, Marek Polacek <polacek@redhat.com> wrote:
> > On Wed, Apr 20, 2016 at 11:04:08AM +0200, Richard Biener wrote:
> >> On Tue, Apr 19, 2016 at 8:35 PM, Marek Polacek <polacek@redhat.com> wrote:
> >> > While predicate_mem_writes has a check to skip conditions that were evaluated
> >> > to true, it's lacking the same check for false, so we hit an assert later on.
> >> > So I'm adding is_false_predicate.  Maybe it should be added to other spots as
> >> > well, but I'm not sure about that.
> >> >
> >> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> >>
> >> Ok.
> >
> > Thanks, should I backport this to gcc-6-branch now?  Or wait until after 6.1?
> 
> It's fine to backport now as it's probably a regression.

Yes, it is (gcc5 worked).  Will backport now then.

	Marek
Jakub Jelinek April 20, 2016, 10:37 a.m. UTC | #5 
On Wed, Apr 20, 2016 at 11:04:08AM +0200, Richard Biener wrote:
> > --- gcc/tree-if-conv.c
> > +++ gcc/tree-if-conv.c
> > @@ -262,6 +262,16 @@ ifc_temp_var (tree type, tree expr, gimple_stmt_iterator *gsi)
> >    return new_name;
> >  }
> >
> > +/* Return true when COND is a false predicate.  */
> > +
> > +static inline bool
> > +is_false_predicate (tree cond)
> > +{
> > +  return (cond == NULL_TREE
> > +         || cond == boolean_false_node
> > +         || integer_zerop (cond));
> > +}
> > +

Is it really a good idea to return true even for cond == NULL_TREE?
I mean it is then very confusing, because both is_true_predicate and
is_false_predicate are true in that case.
It doesn't make a difference when both are used in ||, but looks really
weird and makes the occassional reader wonder if NULL_TREE is valid there at
all and what exactly it means.

> >  /* Return true when COND is a true predicate.  */
> >
> >  static inline bool
> > @@ -1988,7 +1998,7 @@ predicate_mem_writes (loop_p loop)
> >        gimple *stmt;
> >        int index;
> >
> > -      if (is_true_predicate (cond))
> > +      if (is_true_predicate (cond) || is_false_predicate (cond))
> >         continue;
> >
> >        swap = false;
> >
> >         Marek

	Jakub
Richard Biener April 20, 2016, 10:54 a.m. UTC | #6 
On Wed, Apr 20, 2016 at 12:37 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Apr 20, 2016 at 11:04:08AM +0200, Richard Biener wrote:
>> > --- gcc/tree-if-conv.c
>> > +++ gcc/tree-if-conv.c
>> > @@ -262,6 +262,16 @@ ifc_temp_var (tree type, tree expr, gimple_stmt_iterator *gsi)
>> >    return new_name;
>> >  }
>> >
>> > +/* Return true when COND is a false predicate.  */
>> > +
>> > +static inline bool
>> > +is_false_predicate (tree cond)
>> > +{
>> > +  return (cond == NULL_TREE
>> > +         || cond == boolean_false_node
>> > +         || integer_zerop (cond));
>> > +}
>> > +
>
> Is it really a good idea to return true even for cond == NULL_TREE?
> I mean it is then very confusing, because both is_true_predicate and
> is_false_predicate are true in that case.

Ah, indeed.  NULL_TREE is true, not false.

> It doesn't make a difference when both are used in ||, but looks really
> weird and makes the occassional reader wonder if NULL_TREE is valid there at
> all and what exactly it means.
>
>> >  /* Return true when COND is a true predicate.  */
>> >
>> >  static inline bool
>> > @@ -1988,7 +1998,7 @@ predicate_mem_writes (loop_p loop)
>> >        gimple *stmt;
>> >        int index;
>> >
>> > -      if (is_true_predicate (cond))
>> > +      if (is_true_predicate (cond) || is_false_predicate (cond))
>> >         continue;
>> >
>> >        swap = false;
>> >
>> >         Marek
>
>         Jakub
diff mbox

Patch

diff --git gcc/testsuite/gcc.dg/pr70725.c gcc/testsuite/gcc.dg/pr70725.c
index e69de29..fc7b674 100644
--- gcc/testsuite/gcc.dg/pr70725.c
+++ gcc/testsuite/gcc.dg/pr70725.c
@@ -0,0 +1,22 @@ 
+/* PR tree-optimization/70725 */
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+/* { dg-additional-options "-march=skylake-avx512" { target { i?86-*-* x86_64-*-* } } } */
+
+extern short a;
+extern int b, d;
+extern int c[100];
+extern int e;
+extern int f;
+
+void
+fn1 ()
+{
+  for (; e < 2; e = e + 1)
+    d = a;
+  for (;;)
+    for (int g = 0; g < 5; g = g + 1)
+      for (int h = 0; h < 2; h = h + 1)
+	for (int i = 0; i < 3; i = i + 1)
+	  c[f + i] = a && b;
+}
diff --git gcc/tree-if-conv.c gcc/tree-if-conv.c
index 9e305c7..a9fbab9 100644
--- gcc/tree-if-conv.c
+++ gcc/tree-if-conv.c
@@ -262,6 +262,16 @@  ifc_temp_var (tree type, tree expr, gimple_stmt_iterator *gsi)
   return new_name;
 }
 
+/* Return true when COND is a false predicate.  */
+
+static inline bool
+is_false_predicate (tree cond)
+{
+  return (cond == NULL_TREE
+	  || cond == boolean_false_node
+	  || integer_zerop (cond));
+}
+
 /* Return true when COND is a true predicate.  */
 
 static inline bool
@@ -1988,7 +1998,7 @@  predicate_mem_writes (loop_p loop)
       gimple *stmt;
       int index;
 
-      if (is_true_predicate (cond))
+      if (is_true_predicate (cond) || is_false_predicate (cond))
 	continue;
 
       swap = false;