Re: [PATCH PR71503/PR71683]Fix ICE in tree-if-conv.c

[Jeff Law]

On 07/19/2016 07:39 AM, Bin.Cheng wrote:

On Thu, Jul 14, 2016 at 6:49 PM, Jeff Law  wrote:

On 07/14/2016 10:12 AM, Bin Cheng wrote:


Hi,
This is a simple patch fixing ICE in tree-if-conv.c.  Existing code does
not setup a variable (cond) when predicate of basic block is true and it
asserts on the variable.  Interesting thing is dead code is not cleaned up
before ifcvt, but that's another story.
Bootstrap and test on x86_64.  Is it OK?

Thanks,
bin

2016-07-13  Bin Cheng  

PR tree-optimization/71503
PR tree-optimization/71683
* tree-if-conv.c (gen_phi_arg_condition): Set cond when predicate
is true.


Maybe I'm missing something, but in the case where we COND is already set
and we encounter a true predicate later, shouldn't that make the result true
as well?

I don't think the code will though -- we just throw away the true condition.
ISTM that the right thing to do is

if (is_true_predicate (c))
  {
cond = c;
continue;
  }

Can you see if you can trigger a case where we have an existing cond, then
later find a true condition to verify the right thing happens?

Hi,
Attachment is the updated patch, it breaks the loop once TRUE
predicate is found.  I also built spec2k6/spec2k and your case is not
found.  Is it OK?

This version is OK.  Thanks,
jeff

Re: [PATCH PR71503/PR71683]Fix ICE in tree-if-conv.c

[Bin.Cheng]

On Thu, Jul 14, 2016 at 6:49 PM, Jeff Law  wrote:
> On 07/14/2016 10:12 AM, Bin Cheng wrote:
>>
>> Hi,
>> This is a simple patch fixing ICE in tree-if-conv.c.  Existing code does
>> not setup a variable (cond) when predicate of basic block is true and it
>> asserts on the variable.  Interesting thing is dead code is not cleaned up
>> before ifcvt, but that's another story.
>> Bootstrap and test on x86_64.  Is it OK?
>>
>> Thanks,
>> bin
>>
>> 2016-07-13  Bin Cheng  
>>
>> PR tree-optimization/71503
>> PR tree-optimization/71683
>> * tree-if-conv.c (gen_phi_arg_condition): Set cond when predicate
>> is true.
>
> Maybe I'm missing something, but in the case where we COND is already set
> and we encounter a true predicate later, shouldn't that make the result true
> as well?
>
> I don't think the code will though -- we just throw away the true condition.
> ISTM that the right thing to do is
>
> if (is_true_predicate (c))
>   {
> cond = c;
> continue;
>   }
>
> Can you see if you can trigger a case where we have an existing cond, then
> later find a true condition to verify the right thing happens?
Hi,
Attachment is the updated patch, it breaks the loop once TRUE
predicate is found.  I also built spec2k6/spec2k and your case is not
found.  Is it OK?

Thanks,
bin
diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c
index e5a3372..4253d19 100644
--- a/gcc/tree-if-conv.c
+++ b/gcc/tree-if-conv.c
@@ -1687,7 +1687,10 @@ gen_phi_arg_condition (gphi *phi, vec *occur,
   e = gimple_phi_arg_edge (phi, (*occur)[i]);
   c = bb_predicate (e->src);
   if (is_true_predicate (c))
-   continue;
+   {
+ cond = c;
+ break;
+   }
   c = force_gimple_operand_gsi_1 (gsi, unshare_expr (c),
  is_gimple_condexpr, NULL_TREE,
  true, GSI_SAME_STMT);
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr71503.c 
b/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr71503.c
new file mode 100644
index 000..5a90abf
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr71503.c
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-Ofast" { target *-*-* } } */
+
+int a, b;
+unsigned long d;
+void fn1() {
+  unsigned long *h = 
+line1 : {
+  int i = 4;
+  for (; b; i++) {
+d = ((d + 6 ?: *h) ? a : 7) && (i &= 0 >= b);
+b += a;
+  }
+}
+  h = 0;
+  for (; *h;)
+goto line1;
+}
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr71683.c 
b/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr71683.c
new file mode 100644
index 000..851be37
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr71683.c
@@ -0,0 +1,25 @@
+/* { dg-do compile } */
+/* { dg-options "-Ofast" { target *-*-* } } */
+
+short unsigned int ve;
+
+void
+u1 (void)
+{
+  int oq = 0;
+
+  while (ve != 0)
+{
+  int j4, w7 = oq;
+
+  oq = 0 / oq;
+  ve %= oq;
+  j4 = ve ^ 1;
+  ve ^= oq;
+  if (j4 != 0 ? j4 : ve)
+oq = ve;
+  else
+if (w7 != 0)
+  oq = ve;
+}
+}

Re: [PATCH PR71503/PR71683]Fix ICE in tree-if-conv.c

[Bin.Cheng]

On Thu, Jul 14, 2016 at 6:49 PM, Jeff Law  wrote:
> On 07/14/2016 10:12 AM, Bin Cheng wrote:
>>
>> Hi,
>> This is a simple patch fixing ICE in tree-if-conv.c.  Existing code does
>> not setup a variable (cond) when predicate of basic block is true and it
>> asserts on the variable.  Interesting thing is dead code is not cleaned up
>> before ifcvt, but that's another story.
>> Bootstrap and test on x86_64.  Is it OK?
>>
>> Thanks,
>> bin
>>
>> 2016-07-13  Bin Cheng  
>>
>> PR tree-optimization/71503
>> PR tree-optimization/71683
>> * tree-if-conv.c (gen_phi_arg_condition): Set cond when predicate
>> is true.
>
> Maybe I'm missing something, but in the case where we COND is already set
> and we encounter a true predicate later, shouldn't that make the result true
> as well?
Yes, this is my understanding too, if the case does exist.  To my
understanding, conditions for phi arguments should be complimentary to
others.  If one argument has true predicate, then other must? be
false, then the case doesn't exist.

>
> I don't think the code will though -- we just throw away the true condition.
> ISTM that the right thing to do is
>
> if (is_true_predicate (c))
>   {
> cond = c;
> continue;
>   }
Anyway, I think we can make it more robust/efficient by using:
if (is_true_predicate (c))
 {
   cond = c;
   break;
  }
What do you think?  Actually we should discard other arguments if one
has true predicate.

>
> Can you see if you can trigger a case where we have an existing cond, then
> later find a true condition to verify the right thing happens?
Failed to do so.  It should be quite rare, if no impossible,

Thanks,
bin

Re: [PATCH PR71503/PR71683]Fix ICE in tree-if-conv.c

[Jeff Law]

On 07/14/2016 10:12 AM, Bin Cheng wrote:

Hi,
This is a simple patch fixing ICE in tree-if-conv.c.  Existing code does not 
setup a variable (cond) when predicate of basic block is true and it asserts on 
the variable.  Interesting thing is dead code is not cleaned up before ifcvt, 
but that's another story.
Bootstrap and test on x86_64.  Is it OK?

Thanks,
bin

2016-07-13  Bin Cheng  

PR tree-optimization/71503
PR tree-optimization/71683
* tree-if-conv.c (gen_phi_arg_condition): Set cond when predicate
is true.
Maybe I'm missing something, but in the case where we COND is already 
set and we encounter a true predicate later, shouldn't that make the 
result true as well?


I don't think the code will though -- we just throw away the true 
condition.  ISTM that the right thing to do is


if (is_true_predicate (c))
  {
cond = c;
continue;
  }

Can you see if you can trigger a case where we have an existing cond, 
then later find a true condition to verify the right thing happens?


jeff

[PATCH PR71503/PR71683]Fix ICE in tree-if-conv.c

[Bin Cheng]

Hi,
This is a simple patch fixing ICE in tree-if-conv.c.  Existing code does not 
setup a variable (cond) when predicate of basic block is true and it asserts on 
the variable.  Interesting thing is dead code is not cleaned up before ifcvt, 
but that's another story.
Bootstrap and test on x86_64.  Is it OK?

Thanks,
bin

2016-07-13  Bin Cheng  

PR tree-optimization/71503
PR tree-optimization/71683
* tree-if-conv.c (gen_phi_arg_condition): Set cond when predicate
is true.

gcc/testsuite/ChangeLog
2016-07-13  Bin Cheng  

PR tree-optimization/71503
PR tree-optimization/71683
* gcc.dg/tree-ssa/ifc-pr71503.c: New test.
* gcc.dg/tree-ssa/ifc-pr71683.c: New test.
diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c
index e5a3372..3dce0de 100644
--- a/gcc/tree-if-conv.c
+++ b/gcc/tree-if-conv.c
@@ -1687,7 +1687,11 @@ gen_phi_arg_condition (gphi *phi, vec *occur,
   e = gimple_phi_arg_edge (phi, (*occur)[i]);
   c = bb_predicate (e->src);
   if (is_true_predicate (c))
-   continue;
+   {
+ if (!cond)
+   cond = c;
+ continue;
+   }
   c = force_gimple_operand_gsi_1 (gsi, unshare_expr (c),
  is_gimple_condexpr, NULL_TREE,
  true, GSI_SAME_STMT);
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr71503.c 
b/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr71503.c
new file mode 100644
index 000..5a90abf
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr71503.c
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-Ofast" { target *-*-* } } */
+
+int a, b;
+unsigned long d;
+void fn1() {
+  unsigned long *h = 
+line1 : {
+  int i = 4;
+  for (; b; i++) {
+d = ((d + 6 ?: *h) ? a : 7) && (i &= 0 >= b);
+b += a;
+  }
+}
+  h = 0;
+  for (; *h;)
+goto line1;
+}
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr71683.c 
b/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr71683.c
new file mode 100644
index 000..851be37
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/ifc-pr71683.c
@@ -0,0 +1,25 @@
+/* { dg-do compile } */
+/* { dg-options "-Ofast" { target *-*-* } } */
+
+short unsigned int ve;
+
+void
+u1 (void)
+{
+  int oq = 0;
+
+  while (ve != 0)
+{
+  int j4, w7 = oq;
+
+  oq = 0 / oq;
+  ve %= oq;
+  j4 = ve ^ 1;
+  ve ^= oq;
+  if (j4 != 0 ? j4 : ve)
+oq = ve;
+  else
+if (w7 != 0)
+  oq = ve;
+}
+}