diff mbox

[RTL-ifcvt] PR rtl-optimization/68841: Make sure one basic block doesn't clobber CC reg usage of the other

Message ID 56740520.30908@foss.arm.com
State New
Headers show

Commit Message

Kyrill Tkachov Dec. 18, 2015, 1:07 p.m. UTC
Hi all,

In this PR we have a THEN and an ELSE block where one uses the condition reg from the preceeding comparison
but the other block has an arithmetic operation that clobbers the CC reg.
ifcvt emits the latter first and dead code elimination later sees this and eliminates the first comparison
because it sees that the CC reg is clobbered between the comparison and its usage.
That is it generates something like:
(insn 22 21 73 7 (set (reg:CCGC 17 flags)
         (compare:CCGC (reg/v:SI 88 [ i ])
             (const_int 3 [0x3]))) file.c:19 7 {*cmpsi_1}
      (nil))
(insn 73 22 74 7 (parallel [
             (set (reg:SI 94)
                 (ashift:SI (reg/v:SI 88 [ i ])
                     (const_int 1 [0x1])))
             (clobber (reg:CC 17 flags))
         ]) file.c:20 525 {*ashlsi3_1}
      (nil))
(insn 74 73 75 7 (set (reg/v:SI 93 [ k ])
         (eq:SI (reg:CCGC 17 flags)
             (const_int 0 [0]))) file.c:20 626 {*setcc_si_1_movzbl}
      (nil))
(insn 75 74 76 7 (set (reg:SI 95)
         (plus:SI (ashift:SI (reg/v:SI 93 [ k ])
                 (const_int 2 [0x2]))
             (const_int 8 [0x8]))) file.c:20 212 {*leasi}
      (nil))
(insn 76 75 77 7 (set (reg:CCGC 17 flags)
         (compare:CCGC (reg/v:SI 88 [ i ])
             (const_int 4 [0x4]))) file.c:20 7 {*cmpsi_1}
      (nil))
(insn 77 76 33 7 (set (reg/v:SI 87 [ k ])
         (if_then_else:SI (lt (reg:CCGC 17 flags)
                 (const_int 0 [0]))
             (reg:SI 95)
             (reg:SI 94))) file.c:20 966 {*movsicc_noc}
      (nil))

Here insn 74 uses the condition register that the comparison at insn 22 generates but
insn 73 is inserted in between them and DCE later deletes insn 22.

The proposed solution in this patch is to look at the two basic blocks and record whether
they use or clobber the condition reg and then emit the block that uses the CC register first
(if possible).

With this patch the testcase works fine for me on x86_64 and I see no codegen differences
otherwise for SPEC2006 on aarch64 and x86_64.

Bootstrapped and tested on arm, aarch64, x86_64.

Ok for trunk?

Thanks,
Kyrill


2015-12-18  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/68841
     * ifcvt.c (cond_exec_get_condition): Rename to...
     (get_condition_from_jump): ... This.
     (cond_exec_process_if_block): Update callsites.
     (dead_or_predicable): Likewise.
     (noce_record_cc_use_in_bb): New function.
     (noce_try_cmove_arith): Check CC reg usage in both blocks
     and emit them in such an order so as not to clobber the CC reg
     before its use, if possible.

2015-12-18  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/68841
     * gcc.dg/pr68841.c: New test.

Comments

Bernd Schmidt Dec. 18, 2015, 1:16 p.m. UTC | #1
On 12/18/2015 02:07 PM, Kyrill Tkachov wrote:
> In this PR we have a THEN and an ELSE block where one uses the condition
> reg from the preceeding comparison
> but the other block has an arithmetic operation that clobbers the CC reg.
> ifcvt emits the latter first and dead code elimination later sees this
> and eliminates the first comparison
> because it sees that the CC reg is clobbered between the comparison and
> its usage.

>      (noce_try_cmove_arith): Check CC reg usage in both blocks
>      and emit them in such an order so as not to clobber the CC reg
>      before its use, if possible.

Why is this done here? It looks to me like bbs_ok_for_cmove_arith is the 
function that already tries to sort out issues like this. Does it maybe 
just need to be extended to see clobbers?


Bernd
Kyrill Tkachov Dec. 18, 2015, 1:57 p.m. UTC | #2
On 18/12/15 13:16, Bernd Schmidt wrote:
> On 12/18/2015 02:07 PM, Kyrill Tkachov wrote:
>> In this PR we have a THEN and an ELSE block where one uses the condition
>> reg from the preceeding comparison
>> but the other block has an arithmetic operation that clobbers the CC reg.
>> ifcvt emits the latter first and dead code elimination later sees this
>> and eliminates the first comparison
>> because it sees that the CC reg is clobbered between the comparison and
>> its usage.
>
>>      (noce_try_cmove_arith): Check CC reg usage in both blocks
>>      and emit them in such an order so as not to clobber the CC reg
>>      before its use, if possible.
>
> Why is this done here? It looks to me like bbs_ok_for_cmove_arith is the function that already tries to sort out issues like this. Does it maybe just need to be extended to see clobbers?
>

I can do that. A prototype of that approach works for this testacase.
I'll test more extensively...

Thanks,
Kyrill

>
> Bernd
>
diff mbox

Patch

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 2e1fe90e28c4a03b8c1287a45e7f12868aa7684e..e4d695ab72d40f4020b21d9982a9bb5a9b2bae83 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -84,7 +84,7 @@  static rtx_insn *find_active_insn_after (basic_block, rtx_insn *);
 static basic_block block_fallthru (basic_block);
 static int cond_exec_process_insns (ce_if_block *, rtx_insn *, rtx, rtx, int,
 				    int);
-static rtx cond_exec_get_condition (rtx_insn *);
+static rtx get_condition_from_jump (rtx_insn *);
 static rtx noce_get_condition (rtx_insn *, rtx_insn **, bool);
 static int noce_operand_ok (const_rtx);
 static void merge_if_block (ce_if_block *);
@@ -421,7 +421,7 @@  cond_exec_process_insns (ce_if_block *ce_info ATTRIBUTE_UNUSED,
 /* Return the condition for a jump.  Do not do any special processing.  */
 
 static rtx
-cond_exec_get_condition (rtx_insn *jump)
+get_condition_from_jump (rtx_insn *jump)
 {
   rtx test_if, cond;
 
@@ -493,7 +493,7 @@  cond_exec_process_if_block (ce_if_block * ce_info,
 
   /* Find the conditional jump to the ELSE or JOIN part, and isolate
      the test.  */
-  test_expr = cond_exec_get_condition (BB_END (test_bb));
+  test_expr = get_condition_from_jump (BB_END (test_bb));
   if (! test_expr)
     return FALSE;
 
@@ -652,7 +652,7 @@  cond_exec_process_if_block (ce_if_block * ce_info,
 	    goto fail;
 
 	  /* Find the conditional jump and isolate the test.  */
-	  t = cond_exec_get_condition (BB_END (bb));
+	  t = get_condition_from_jump (BB_END (bb));
 	  if (! t)
 	    goto fail;
 
@@ -2017,6 +2017,29 @@  noce_emit_bb (rtx last_insn, basic_block bb, bool simple)
   return true;
 }
 
+/* For basic block BB record in USES_CC and CLOBBERS_CC whether it
+   uses and/or clobbers the condition code register CC.  */
+
+static void
+noce_record_cc_use_in_bb (basic_block bb, rtx cc,
+			  bool *uses_cc, bool *clobbers_cc)
+{
+  rtx_insn *insn = NULL;
+  *uses_cc = false;
+  *clobbers_cc = false;
+  if (!cc)
+    return;
+  FOR_BB_INSNS (bb, insn)
+    {
+      if (!active_insn_p (insn))
+	continue;
+
+      *clobbers_cc |= set_of (cc, PATTERN (insn)) != NULL_RTX;
+      *uses_cc |= reg_mentioned_p (cc, SET_SRC (single_set (insn)));
+    }
+}
+
+
 /* Try more complex cases involving conditional_move.  */
 
 static int
@@ -2202,6 +2225,26 @@  noce_try_cmove_arith (struct noce_if_info *if_info)
 	}
     }
 
+  bool then_uses_cc = false;
+  bool then_clobbers_cc = false;
+  bool else_uses_cc = false;
+  bool else_clobbers_cc = false;
+
+  rtx cc = cc_in_cond (if_info->cond);
+  /* If there is no condition code register in cond
+     (noce_get_condition could have replaced it) look into the original
+     jump condition.  */
+  if (!cc)
+    {
+      rtx jmp_cond = get_condition_from_jump (if_info->jump);
+      if (jmp_cond)
+	cc = cc_in_cond (jmp_cond);
+    }
+  if (then_bb)
+    noce_record_cc_use_in_bb (then_bb, cc, &then_uses_cc, &then_clobbers_cc);
+  if (else_bb)
+    noce_record_cc_use_in_bb (else_bb, cc, &else_uses_cc, &else_clobbers_cc);
+
   modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
   if (tmp_b && then_bb)
     {
@@ -2233,8 +2276,10 @@  noce_try_cmove_arith (struct noce_if_info *if_info)
 
   /* If insn to set up A clobbers any registers B depends on, try to
      swap insn that sets up A with the one that sets up B.  If even
-     that doesn't help, punt.  */
-  if (modified_in_a && !modified_in_b)
+     that doesn't help, punt.  Make sure one basic block doesn't clobber
+     the condition code register before the other uses it.  */
+  if (modified_in_a && !modified_in_b
+      && !(then_uses_cc && else_clobbers_cc))
     {
       if (!noce_emit_bb (emit_b, else_bb, b_simple))
 	goto end_seq_and_fail;
@@ -2242,7 +2287,8 @@  noce_try_cmove_arith (struct noce_if_info *if_info)
       if (!noce_emit_bb (emit_a, then_bb, a_simple))
 	goto end_seq_and_fail;
     }
-  else if (!modified_in_a)
+  else if (!modified_in_a
+	   && !(else_uses_cc && then_clobbers_cc))
     {
       if (!noce_emit_bb (emit_a, then_bb, a_simple))
 	goto end_seq_and_fail;
@@ -5045,7 +5091,7 @@  dead_or_predicable (basic_block test_bb, basic_block merge_bb,
 
       rtx cond;
 
-      cond = cond_exec_get_condition (jump);
+      cond = get_condition_from_jump (jump);
       if (! cond)
 	return FALSE;
 
diff --git a/gcc/testsuite/gcc.dg/pr68841.c b/gcc/testsuite/gcc.dg/pr68841.c
new file mode 100644
index 0000000000000000000000000000000000000000..470048cc24f0d7150ed1e3141181bc1e8472ae12
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr68841.c
@@ -0,0 +1,34 @@ 
+/* { dg-do run } */
+/* { dg-options "-Og -fif-conversion -flive-range-shrinkage -fpeel-loops -frerun-cse-after-loop" } */
+
+static inline int
+foo (int *x, int y)
+{
+  int z = *x;
+  while (y > z)
+    z *= 2;
+  return z;
+}
+
+int
+main ()
+{
+  int i;
+  for (i = 1; i < 17; i++)
+    {
+      int j;
+      int k;
+      j = foo (&i, 7);
+      if (i >= 7)
+	k = i;
+      else if (i >= 4)
+	k = 8 + (i - 4) * 2;
+      else if (i == 3)
+	k = 12;
+      else
+	k = 8;
+      if (j != k)
+	__builtin_abort ();
+    }
+  return 0;
+}