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

[Kyrill Tkachov <kyrylo dot tkachov at arm dot com>]

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

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