[PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

[PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]

Hi all,

In this PR we have an IF-THEN-JOIN formation i.e. no ELSE block and we have a situation
where the THEN block modifies a register used in emit_b, so emit_b must be emitted before
the THEN block. However the bug in the logic that performs these checks ends up to us
emitting emit_a+then_bb followed by emit_b+else_bb.

The fix is pretty simple and involves emitting emit_b (+ else_bb that is empty in this case)
if modified_a is true, even if emit_a is NULL. If emit_a is NULL noce_emit_bb will handle
it properly and not do anything bad, so we're safe.

Bootstrapped and tested on arm, aarch64, x86_64.

Ok for trunk?

Thanks,
Kyrill

2015-11-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/68506
     * ifcvt.c (noce_try_cmove_arith): Try emitting the else basic block
     first if emit_a exists or then_bb modifies 'b'.

2015-11-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/68506
     * gcc.c-torture/execute/pr68506.c: New test.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ifcvt-if-then-join.patch --]
[-- Type: text/x-patch; name=ifcvt-if-then-join.patch, Size: 1814 bytes --]

commit 08a371d20793bd57e9a68b85beaf2cab0804ed48
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Tue Nov 24 11:49:30 2015 +0000

    PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index af7a3b9..3e3dc8d 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2220,7 +2220,7 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
 	  }
 
     }
-    if (emit_a && modified_in_a)
+    if (emit_a || modified_in_a)
       {
 	modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
 	if (tmp_b && else_bb)
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68506.c b/gcc/testsuite/gcc.c-torture/execute/pr68506.c
new file mode 100644
index 0000000..15984ed
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68506.c
@@ -0,0 +1,63 @@
+/* { dg-options "-fno-builtin-abort" } */
+
+int a, b, m, n, o, p, s, u, i;
+char c, q, y;
+short d;
+unsigned char e;
+static int f, h;
+static short g, r, v;
+unsigned t;
+
+extern void abort ();
+
+int
+fn1 (int p1)
+{
+  return a ? p1 : p1 + a;
+}
+
+unsigned char
+fn2 (unsigned char p1, int p2)
+{
+  return p2 >= 2 ? p1 : p1 >> p2;
+}
+
+static short
+fn3 ()
+{
+  int w, x = 0;
+  for (; p < 31; p++)
+    {
+      s = fn1 (c | ((1 && c) == c));
+      t = fn2 (s, x);
+      c = (unsigned) c > -(unsigned) ((o = (m = d = t) == p) <= 4UL) && n;
+      v = -c;
+      y = 1;
+      for (; y; y++)
+	e = v == 1;
+      d = 0;
+      for (; h != 2;)
+	{
+	  for (;;)
+	    {
+	      if (!m)
+		abort ();
+	      r = 7 - f;
+	      x = e = i | r;
+	      q = u * g;
+	      w = b == q;
+	      if (w)
+		break;
+	    }
+	  break;
+	}
+    }
+  return x;
+}
+
+int
+main ()
+{
+  fn3 ();
+  return 0;
+}

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

[Bernd Schmidt]

On 11/26/2015 12:12 PM, Kyrill Tkachov wrote:
> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
> index af7a3b9..3e3dc8d 100644
> --- a/gcc/ifcvt.c
> +++ b/gcc/ifcvt.c
> @@ -2220,7 +2220,7 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
>   	  }
>
>       }
> -    if (emit_a && modified_in_a)
> +    if (emit_a || modified_in_a)
>         {
Having stared at it in the debugger for a while, I think I managed to 
convince myself that this is correct. So, OK.

A few other comments. This whole if block is indented too far, please 
fix while you're there. Also eliminate the unnecessary blank lines 
before closing braces (two instances inside this if block). There are 
other formatting errors in this function, but those are best left alone 
for now.

>   	modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);

Can this ever be true? We arrange for emit_b to set a new pseudo, don't 
we? Are we allowing cases where we copy a pattern that sets more than 
one register, and is that safe?


Bernd

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

[Kyrill Tkachov]

On 26/11/15 13:40, Bernd Schmidt wrote:
> On 11/26/2015 12:12 PM, Kyrill Tkachov wrote:
>> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
>> index af7a3b9..3e3dc8d 100644
>> --- a/gcc/ifcvt.c
>> +++ b/gcc/ifcvt.c
>> @@ -2220,7 +2220,7 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
>>         }
>>
>>       }
>> -    if (emit_a && modified_in_a)
>> +    if (emit_a || modified_in_a)
>>         {
> Having stared at it in the debugger for a while, I think I managed to convince myself that this is correct. So, OK.
>

Thanks.

> A few other comments. This whole if block is indented too far, please fix while you're there. Also eliminate the unnecessary blank lines before closing braces (two instances inside this if block). There are other formatting errors in this 
> function, but those are best left alone for now.

Ok, I'll fix the indentation in that if-else block

>
>>       modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
>
> Can this ever be true? We arrange for emit_b to set a new pseudo, don't we? Are we allowing cases where we copy a pattern that sets more than one register, and is that safe?
>

You're right, this statement always sets modifieb_in_b to false. We reject anything bug single_set insns
by this point in the code. I'll replace that with modified_in_b = false;

Thanks,
Kyrill
>
> Bernd
>

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

[Bernd Schmidt]

On 11/26/2015 02:52 PM, Kyrill Tkachov wrote:
>
> On 26/11/15 13:40, Bernd Schmidt wrote:
>> On 11/26/2015 12:12 PM, Kyrill Tkachov wrote:
>>>       modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a,
>>> emit_b);
>>
>> Can this ever be true? We arrange for emit_b to set a new pseudo,
>> don't we? Are we allowing cases where we copy a pattern that sets more
>> than one register, and is that safe?
>
> You're right, this statement always sets modifieb_in_b to false. We
> reject anything bug single_set insns
> by this point in the code. I'll replace that with modified_in_b = false;

Note that there's a mirrored test for modified_in_a, and both are 
already initialized to false. Also - careful with single_set, it can 
return true even for multiple sets in case there's a REG_DEAD note on 
one of them. You might want to strengthen your tests to also include 
!multiple_sets. Then, maybe instead of deleting these tests, turn them 
into gcc_checking_asserts.


Bernd

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

[Kyrill Tkachov]

On 26/11/15 14:23, Bernd Schmidt wrote:
> On 11/26/2015 02:52 PM, Kyrill Tkachov wrote:
>>
>> On 26/11/15 13:40, Bernd Schmidt wrote:
>>> On 11/26/2015 12:12 PM, Kyrill Tkachov wrote:
>>>>       modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a,
>>>> emit_b);
>>>
>>> Can this ever be true? We arrange for emit_b to set a new pseudo,
>>> don't we? Are we allowing cases where we copy a pattern that sets more
>>> than one register, and is that safe?
>>
>> You're right, this statement always sets modifieb_in_b to false. We
>> reject anything bug single_set insns
>> by this point in the code. I'll replace that with modified_in_b = false;
>
> Note that there's a mirrored test for modified_in_a, and both are already initialized to false.

Yeah, that can be changed to just false too. I'll do that in the next revision.

> Also - careful with single_set, it can return true even for multiple sets in case there's a REG_DEAD note on one of them. You might want to strengthen your tests to also include !multiple_sets. Then, maybe instead of deleting these tests, 
> turn them into gcc_checking_asserts.
>

I see. I think the best place to do that would be in insn_valid_noce_process_p and just get it to return
false if multiple_sets (insn) is true.

Would it be ok if I did that as a separate follow-up patch?
We don't have a testcase where this actually causes trouble and I'd like to keep the fix for
this PR as self-contained as possible.

Thanks,
Kyrill



>
> Bernd
>

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

[Bernd Schmidt]

On 11/26/2015 03:35 PM, Kyrill Tkachov wrote:
> Would it be ok if I did that as a separate follow-up patch?
> We don't have a testcase where this actually causes trouble and I'd like
> to keep the fix for
> this PR as self-contained as possible.

Sure.


Bernd

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 2227 bytes --]


On 26/11/15 14:35, Kyrill Tkachov wrote:
>
> On 26/11/15 14:23, Bernd Schmidt wrote:
>> On 11/26/2015 02:52 PM, Kyrill Tkachov wrote:
>>>
>>> On 26/11/15 13:40, Bernd Schmidt wrote:
>>>> On 11/26/2015 12:12 PM, Kyrill Tkachov wrote:
>>>>>       modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a,
>>>>> emit_b);
>>>>
>>>> Can this ever be true? We arrange for emit_b to set a new pseudo,
>>>> don't we? Are we allowing cases where we copy a pattern that sets more
>>>> than one register, and is that safe?
>>>
>>> You're right, this statement always sets modifieb_in_b to false. We
>>> reject anything bug single_set insns
>>> by this point in the code. I'll replace that with modified_in_b = false;
>>
>> Note that there's a mirrored test for modified_in_a, and both are already initialized to false.
>
> Yeah, that can be changed to just false too. I'll do that in the next revision.
>

Here is the updated patch.
I've reindented the if-else blocks and removed the always-false initialisation of modified_a and modified_b.

Re-tested on x86_64, aarch64.

Ok for trunk?

Thanks,
Kyrill

2015-11-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/68506
     * ifcvt.c (noce_try_cmove_arith): Try emitting the else basic block
     first if emit_a exists or then_bb modifies 'b'.  Reindent if-else
     blocks.

2015-11-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/68506
     * gcc.c-torture/execute/pr68506.c: New test.


>> Also - careful with single_set, it can return true even for multiple sets in case there's a REG_DEAD note on one of them. You might want to strengthen your tests to also include !multiple_sets. Then, maybe instead of deleting these 
>> tests, turn them into gcc_checking_asserts.
>>
>
> I see. I think the best place to do that would be in insn_valid_noce_process_p and just get it to return
> false if multiple_sets (insn) is true.
>
> Would it be ok if I did that as a separate follow-up patch?
> We don't have a testcase where this actually causes trouble and I'd like to keep the fix for
> this PR as self-contained as possible.
>
> Thanks,
> Kyrill
>
>
>
>>
>> Bernd
>>
>


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ifcvt-fix.patch --]
[-- Type: text/x-patch; name=ifcvt-fix.patch, Size: 3690 bytes --]

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index af7a3b96f38bea429f86916fc176913cac2e6ebc..9092b377e45082ec07a30d60b070575f4dc68641 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2206,7 +2206,6 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
        swap insn that sets up A with the one that sets up B.  If even
        that doesn't help, punt.  */
 
-  modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
   if (tmp_b && then_bb)
     {
       FOR_BB_INSNS (then_bb, tmp_insn)
@@ -2220,40 +2219,37 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
 	  }
 
     }
-    if (emit_a && modified_in_a)
-      {
-	modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
-	if (tmp_b && else_bb)
-	  {
-	    FOR_BB_INSNS (else_bb, tmp_insn)
-	    /* Don't check inside insn_b.  We will have changed it to emit_b
-	       with a destination that doesn't conflict.  */
-	      if (!(insn_b && tmp_insn == insn_b)
-		  && modified_in_p (orig_a, tmp_insn))
-		{
-		  modified_in_b = true;
-		  break;
-		}
-
-	  }
-	if (modified_in_b)
-	  goto end_seq_and_fail;
-
-	if (!noce_emit_bb (emit_b, else_bb, b_simple))
-	  goto end_seq_and_fail;
+  if (emit_a || modified_in_a)
+    {
+      if (tmp_b && else_bb)
+	{
+	  FOR_BB_INSNS (else_bb, tmp_insn)
+	  /* Don't check inside insn_b.  We will have changed it to emit_b
+	     with a destination that doesn't conflict.  */
+	  if (!(insn_b && tmp_insn == insn_b)
+	      && modified_in_p (orig_a, tmp_insn))
+	    {
+	      modified_in_b = true;
+	      break;
+	    }
+	}
+      if (modified_in_b)
+	goto end_seq_and_fail;
 
-	if (!noce_emit_bb (emit_a, then_bb, a_simple))
-	  goto end_seq_and_fail;
-      }
-    else
-      {
-	if (!noce_emit_bb (emit_a, then_bb, a_simple))
-	  goto end_seq_and_fail;
+      if (!noce_emit_bb (emit_b, else_bb, b_simple))
+	goto end_seq_and_fail;
 
-	if (!noce_emit_bb (emit_b, else_bb, b_simple))
-	  goto end_seq_and_fail;
+      if (!noce_emit_bb (emit_a, then_bb, a_simple))
+	goto end_seq_and_fail;
+    }
+  else
+    {
+      if (!noce_emit_bb (emit_a, then_bb, a_simple))
+	goto end_seq_and_fail;
 
-      }
+      if (!noce_emit_bb (emit_b, else_bb, b_simple))
+	goto end_seq_and_fail;
+    }
 
   target = noce_emit_cmove (if_info, x, code, XEXP (if_info->cond, 0),
 			    XEXP (if_info->cond, 1), a, b);
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68506.c b/gcc/testsuite/gcc.c-torture/execute/pr68506.c
new file mode 100644
index 0000000000000000000000000000000000000000..15984edfe0812dc1dbbc8a07bc5b95997ed3acb9
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68506.c
@@ -0,0 +1,63 @@
+/* { dg-options "-fno-builtin-abort" } */
+
+int a, b, m, n, o, p, s, u, i;
+char c, q, y;
+short d;
+unsigned char e;
+static int f, h;
+static short g, r, v;
+unsigned t;
+
+extern void abort ();
+
+int
+fn1 (int p1)
+{
+  return a ? p1 : p1 + a;
+}
+
+unsigned char
+fn2 (unsigned char p1, int p2)
+{
+  return p2 >= 2 ? p1 : p1 >> p2;
+}
+
+static short
+fn3 ()
+{
+  int w, x = 0;
+  for (; p < 31; p++)
+    {
+      s = fn1 (c | ((1 && c) == c));
+      t = fn2 (s, x);
+      c = (unsigned) c > -(unsigned) ((o = (m = d = t) == p) <= 4UL) && n;
+      v = -c;
+      y = 1;
+      for (; y; y++)
+	e = v == 1;
+      d = 0;
+      for (; h != 2;)
+	{
+	  for (;;)
+	    {
+	      if (!m)
+		abort ();
+	      r = 7 - f;
+	      x = e = i | r;
+	      q = u * g;
+	      w = b == q;
+	      if (w)
+		break;
+	    }
+	  break;
+	}
+    }
+  return x;
+}
+
+int
+main ()
+{
+  fn3 ();
+  return 0;
+}

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

[Bernd Schmidt]

On 11/26/2015 05:45 PM, Kyrill Tkachov wrote:
>          that doesn't help, punt.  */
>
> -  modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
>     if (tmp_b && then_bb)
>       {
These bits I thought would be part of a followup patch (which would also 
guard against single_set problems), and as I mentioned I'd rather have a 
checking assert.

So take these deletions out and leave them for the followup, and the 
patch is ok everywhere. No need for a full retest given that practically 
the same patch has been tested already, just make sure you can build cc1.


Bernd

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

[Kyrill Tkachov]

On 26/11/15 16:49, Bernd Schmidt wrote:
> On 11/26/2015 05:45 PM, Kyrill Tkachov wrote:
>>          that doesn't help, punt.  */
>>
>> -  modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
>>     if (tmp_b && then_bb)
>>       {
> These bits I thought would be part of a followup patch (which would also guard against single_set problems), and as I mentioned I'd rather have a checking assert.
Yes, you're right. I have the checking_assert statement in the followup that I've been testing.
I'll move the deletion of these two statements there as well to minimise the changes to this patch.

I'll move these bits to that patch, re-build cc1 and commit.

Thanks for your guidance,
Kyrill

> So take these deletions out and leave them for the followup, and the patch is ok everywhere. No need for a full retest given that practically the same patch has been tested already, just make sure you can build cc1.
>
>
> Bernd
>

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1496 bytes --]


On 26/11/15 16:54, Kyrill Tkachov wrote:
>
> On 26/11/15 16:49, Bernd Schmidt wrote:
>> On 11/26/2015 05:45 PM, Kyrill Tkachov wrote:
>>>          that doesn't help, punt.  */
>>>
>>> -  modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
>>>     if (tmp_b && then_bb)
>>>       {
>> These bits I thought would be part of a followup patch (which would also guard against single_set problems), and as I mentioned I'd rather have a checking assert.
> Yes, you're right. I have the checking_assert statement in the followup that I've been testing.
> I'll move the deletion of these two statements there as well to minimise the changes to this patch.
>
> I'll move these bits to that patch, re-build cc1 and commit.
>

Here it is.
I'm committing this to trunk.

Thanks,
Kyrill

2015-11-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/68506
     * ifcvt.c (noce_try_cmove_arith): Try emitting the else basic block
     first if emit_a exists or then_bb modifies 'b'.  Reindent if-else
     blocks.

2015-11-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/68506
     * gcc.c-torture/execute/pr68506.c: New test.

> Thanks for your guidance,
> Kyrill
>
>> So take these deletions out and leave them for the followup, and the patch is ok everywhere. No need for a full retest given that practically the same patch has been tested already, just make sure you can build cc1.
>>
>>
>> Bernd
>>
>


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ifcvt-fix.patch --]
[-- Type: text/x-patch; name=ifcvt-fix.patch, Size: 3522 bytes --]

commit ba7633ec30e8e25d7dc1975893bf56eadf223404
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Tue Nov 24 11:49:30 2015 +0000

    PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index af7a3b9..3ce9fe6 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2220,40 +2220,38 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
 	  }
 
     }
-    if (emit_a && modified_in_a)
-      {
-	modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
-	if (tmp_b && else_bb)
-	  {
-	    FOR_BB_INSNS (else_bb, tmp_insn)
-	    /* Don't check inside insn_b.  We will have changed it to emit_b
-	       with a destination that doesn't conflict.  */
-	      if (!(insn_b && tmp_insn == insn_b)
-		  && modified_in_p (orig_a, tmp_insn))
-		{
-		  modified_in_b = true;
-		  break;
-		}
-
-	  }
-	if (modified_in_b)
-	  goto end_seq_and_fail;
-
-	if (!noce_emit_bb (emit_b, else_bb, b_simple))
-	  goto end_seq_and_fail;
+  if (emit_a || modified_in_a)
+    {
+      modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
+      if (tmp_b && else_bb)
+	{
+	  FOR_BB_INSNS (else_bb, tmp_insn)
+	  /* Don't check inside insn_b.  We will have changed it to emit_b
+	     with a destination that doesn't conflict.  */
+	  if (!(insn_b && tmp_insn == insn_b)
+	      && modified_in_p (orig_a, tmp_insn))
+	    {
+	      modified_in_b = true;
+	      break;
+	    }
+	}
+      if (modified_in_b)
+	goto end_seq_and_fail;
 
-	if (!noce_emit_bb (emit_a, then_bb, a_simple))
-	  goto end_seq_and_fail;
-      }
-    else
-      {
-	if (!noce_emit_bb (emit_a, then_bb, a_simple))
-	  goto end_seq_and_fail;
+      if (!noce_emit_bb (emit_b, else_bb, b_simple))
+	goto end_seq_and_fail;
 
-	if (!noce_emit_bb (emit_b, else_bb, b_simple))
-	  goto end_seq_and_fail;
+      if (!noce_emit_bb (emit_a, then_bb, a_simple))
+	goto end_seq_and_fail;
+    }
+  else
+    {
+      if (!noce_emit_bb (emit_a, then_bb, a_simple))
+	goto end_seq_and_fail;
 
-      }
+      if (!noce_emit_bb (emit_b, else_bb, b_simple))
+	goto end_seq_and_fail;
+    }
 
   target = noce_emit_cmove (if_info, x, code, XEXP (if_info->cond, 0),
 			    XEXP (if_info->cond, 1), a, b);
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68506.c b/gcc/testsuite/gcc.c-torture/execute/pr68506.c
new file mode 100644
index 0000000..15984ed
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68506.c
@@ -0,0 +1,63 @@
+/* { dg-options "-fno-builtin-abort" } */
+
+int a, b, m, n, o, p, s, u, i;
+char c, q, y;
+short d;
+unsigned char e;
+static int f, h;
+static short g, r, v;
+unsigned t;
+
+extern void abort ();
+
+int
+fn1 (int p1)
+{
+  return a ? p1 : p1 + a;
+}
+
+unsigned char
+fn2 (unsigned char p1, int p2)
+{
+  return p2 >= 2 ? p1 : p1 >> p2;
+}
+
+static short
+fn3 ()
+{
+  int w, x = 0;
+  for (; p < 31; p++)
+    {
+      s = fn1 (c | ((1 && c) == c));
+      t = fn2 (s, x);
+      c = (unsigned) c > -(unsigned) ((o = (m = d = t) == p) <= 4UL) && n;
+      v = -c;
+      y = 1;
+      for (; y; y++)
+	e = v == 1;
+      d = 0;
+      for (; h != 2;)
+	{
+	  for (;;)
+	    {
+	      if (!m)
+		abort ();
+	      r = 7 - f;
+	      x = e = i | r;
+	      q = u * g;
+	      w = b == q;
+	      if (w)
+		break;
+	    }
+	  break;
+	}
+    }
+  return x;
+}
+
+int
+main ()
+{
+  fn3 ();
+  return 0;
+}

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

[Richard Biener]

On Fri, Nov 27, 2015 at 10:44 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>
> On 26/11/15 16:54, Kyrill Tkachov wrote:
>>
>>
>> On 26/11/15 16:49, Bernd Schmidt wrote:
>>>
>>> On 11/26/2015 05:45 PM, Kyrill Tkachov wrote:
>>>>
>>>>          that doesn't help, punt.  */
>>>>
>>>> -  modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
>>>>     if (tmp_b && then_bb)
>>>>       {
>>>
>>> These bits I thought would be part of a followup patch (which would also
>>> guard against single_set problems), and as I mentioned I'd rather have a
>>> checking assert.
>>
>> Yes, you're right. I have the checking_assert statement in the followup
>> that I've been testing.
>> I'll move the deletion of these two statements there as well to minimise
>> the changes to this patch.
>>
>> I'll move these bits to that patch, re-build cc1 and commit.
>>
>
> Here it is.
> I'm committing this to trunk.

I think this causes

FAIL: gcc.c-torture/execute/20050124-1.c   -O2  (internal compiler error)
FAIL: gcc.c-torture/execute/20050124-1.c   -O2  (test for excess errors)
WARNING: gcc.c-torture/execute/20050124-1.c   -O2  compilation failed to produce
 executable
FAIL: gcc.c-torture/execute/20050124-1.c   -O2 -flto -fno-use-linker-plugin -flt
o-partition=none  (internal compiler error)
FAIL: gcc.c-torture/execute/20050124-1.c   -O2 -flto -fno-use-linker-plugin -flt
o-partition=none  (test for excess errors)
....

/space/rguenther/src/svn/trunk2/gcc/testsuite/gcc.c-torture/execute/20050124-1.c:19:1:
internal compiler error: in noce_try_cmove_arith, at ifcvt.c:2180^M
0x11f919d noce_try_cmove_arith^M
        /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:2180^M
0x11fb93f noce_process_if_block^M
        /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:3525^M
0x11fdd0e noce_find_if_block^M
        /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:3974^M
0x11fdd0e find_if_header^M
        /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:4179^M
0x11fdd0e if_convert^M
        /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:5326^M
0x11ff32d execute^M


on x86_64 with -m64 and -m32.

Richard.

> Thanks,
> Kyrill
>
> 2015-11-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     PR rtl-optimization/68506
>     * ifcvt.c (noce_try_cmove_arith): Try emitting the else basic block
>     first if emit_a exists or then_bb modifies 'b'.  Reindent if-else
>     blocks.
>
> 2015-11-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     PR rtl-optimization/68506
>     * gcc.c-torture/execute/pr68506.c: New test.
>
>> Thanks for your guidance,
>> Kyrill
>>
>>> So take these deletions out and leave them for the followup, and the
>>> patch is ok everywhere. No need for a full retest given that practically the
>>> same patch has been tested already, just make sure you can build cc1.
>>>
>>>
>>> Bernd
>>>
>>
>

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

[Kyrill Tkachov]

On 27/11/15 14:09, Richard Biener wrote:
> On Fri, Nov 27, 2015 at 10:44 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>> On 26/11/15 16:54, Kyrill Tkachov wrote:
>>>
>>> On 26/11/15 16:49, Bernd Schmidt wrote:
>>>> On 11/26/2015 05:45 PM, Kyrill Tkachov wrote:
>>>>>           that doesn't help, punt.  */
>>>>>
>>>>> -  modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
>>>>>      if (tmp_b && then_bb)
>>>>>        {
>>>> These bits I thought would be part of a followup patch (which would also
>>>> guard against single_set problems), and as I mentioned I'd rather have a
>>>> checking assert.
>>> Yes, you're right. I have the checking_assert statement in the followup
>>> that I've been testing.
>>> I'll move the deletion of these two statements there as well to minimise
>>> the changes to this patch.
>>>
>>> I'll move these bits to that patch, re-build cc1 and commit.
>>>
>> Here it is.
>> I'm committing this to trunk.
> I think this causes
>
> FAIL: gcc.c-torture/execute/20050124-1.c   -O2  (internal compiler error)
> FAIL: gcc.c-torture/execute/20050124-1.c   -O2  (test for excess errors)
> WARNING: gcc.c-torture/execute/20050124-1.c   -O2  compilation failed to produce
>   executable
> FAIL: gcc.c-torture/execute/20050124-1.c   -O2 -flto -fno-use-linker-plugin -flt
> o-partition=none  (internal compiler error)
> FAIL: gcc.c-torture/execute/20050124-1.c   -O2 -flto -fno-use-linker-plugin -flt
> o-partition=none  (test for excess errors)
> ....

Sorry for that.
That is caused not by this patch but rather by the followup
https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03327.html

The checking assert fails:
gcc_checking_assert (!emit_a || !modified_in_p (orig_b, emit_a));
emit_a is:
(parallel [
         (set (reg:SI 93)
             (plus:SI (reg/v:SI 88 [ i ])
                 (const_int 2 [0x2])))
         (clobber (reg:CC 17 flags))
     ])

and and orig_b is:
(if_then_else:SI (eq (reg:CC 17 flags)
         (const_int 0 [0]))
     (reg/v:SI 87 [ <retval> ])
     (reg/v:SI 88 [ i ]))

So I think our assumption that this case would never trigger by this point doesn't hold
due to the CC reg clobber.
So the code before that patch was probably correct.
I think we should revert https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03327.html then.

Kyrill

> /space/rguenther/src/svn/trunk2/gcc/testsuite/gcc.c-torture/execute/20050124-1.c:19:1:
> internal compiler error: in noce_try_cmove_arith, at ifcvt.c:2180^M
> 0x11f919d noce_try_cmove_arith^M
>          /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:2180^M
> 0x11fb93f noce_process_if_block^M
>          /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:3525^M
> 0x11fdd0e noce_find_if_block^M
>          /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:3974^M
> 0x11fdd0e find_if_header^M
>          /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:4179^M
> 0x11fdd0e if_convert^M
>          /space/rguenther/src/svn/trunk2/gcc/ifcvt.c:5326^M
> 0x11ff32d execute^M
>
>
> on x86_64 with -m64 and -m32.
>
> Richard.
>
>> Thanks,
>> Kyrill
>>
>> 2015-11-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>      PR rtl-optimization/68506
>>      * ifcvt.c (noce_try_cmove_arith): Try emitting the else basic block
>>      first if emit_a exists or then_bb modifies 'b'.  Reindent if-else
>>      blocks.
>>
>> 2015-11-26  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>      PR rtl-optimization/68506
>>      * gcc.c-torture/execute/pr68506.c: New test.
>>
>>> Thanks for your guidance,
>>> Kyrill
>>>
>>>> So take these deletions out and leave them for the followup, and the
>>>> patch is ok everywhere. No need for a full retest given that practically the
>>>> same patch has been tested already, just make sure you can build cc1.
>>>>
>>>>
>>>> Bernd
>>>>

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

[Bernd Schmidt]

On 11/27/2015 03:33 PM, Kyrill Tkachov wrote:
> Sorry for that.
> That is caused not by this patch but rather by the followup
> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03327.html
>
> The checking assert fails:
> gcc_checking_assert (!emit_a || !modified_in_p (orig_b, emit_a));
> emit_a is:
> (parallel [
>          (set (reg:SI 93)
>              (plus:SI (reg/v:SI 88 [ i ])
>                  (const_int 2 [0x2])))
>          (clobber (reg:CC 17 flags))
>      ])
>
> and and orig_b is:
> (if_then_else:SI (eq (reg:CC 17 flags)
>          (const_int 0 [0]))
>      (reg/v:SI 87 [ <retval> ])
>      (reg/v:SI 88 [ i ]))
>
> So I think our assumption that this case would never trigger by this
> point doesn't hold
> due to the CC reg clobber.
> So the code before that patch was probably correct.
> I think we should revert
> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03327.html then.

Yes. Sorry. I thought orig_b would hold "normal" objects and not such an 
if-then-else.


Bernd

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/68506: Fix emitting order of insns in IF-THEN-JOIN case

[Kyrill Tkachov]

On 27/11/15 14:35, Bernd Schmidt wrote:
> On 11/27/2015 03:33 PM, Kyrill Tkachov wrote:
>> Sorry for that.
>> That is caused not by this patch but rather by the followup
>> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03327.html
>>
>> The checking assert fails:
>> gcc_checking_assert (!emit_a || !modified_in_p (orig_b, emit_a));
>> emit_a is:
>> (parallel [
>>          (set (reg:SI 93)
>>              (plus:SI (reg/v:SI 88 [ i ])
>>                  (const_int 2 [0x2])))
>>          (clobber (reg:CC 17 flags))
>>      ])
>>
>> and and orig_b is:
>> (if_then_else:SI (eq (reg:CC 17 flags)
>>          (const_int 0 [0]))
>>      (reg/v:SI 87 [ <retval> ])
>>      (reg/v:SI 88 [ i ]))
>>
>> So I think our assumption that this case would never trigger by this
>> point doesn't hold
>> due to the CC reg clobber.
>> So the code before that patch was probably correct.
>> I think we should revert
>> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03327.html then.
>
> Yes. Sorry. I thought orig_b would hold "normal" objects and not such an if-then-else.
>

Reverted with r231019.
Sorry for not catching it myself earlier.

Kyrill

>
> Bernd
>