diff mbox

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

Message ID 5658261A.1030201@arm.com
State New
Headers show

Commit Message

Kyrylo Tkachov Nov. 27, 2015, 9:44 a.m. UTC 
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
>>
>

Comments

Richard Biener Nov. 27, 2015, 2:09 p.m. UTC | #1 
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
>>>
>>
>
Kyrylo Tkachov Nov. 27, 2015, 2:33 p.m. UTC | #2 
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
>>>>
Bernd Schmidt Nov. 27, 2015, 2:35 p.m. UTC | #3 
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
Kyrylo Tkachov Nov. 27, 2015, 2:41 p.m. UTC | #4 
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
>
diff mbox

Patch

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;
+}