Patchwork Fix ifcvt ICE

login
register
mail settings
Submitter Chung-Lin Tang
Date Oct. 11, 2010, 5:49 p.m.
Message ID <4CB34E16.3020804@codesourcery.com>
Download mbox | patch
Permalink /patch/67461/
State New
Headers show

Comments

Chung-Lin Tang - Oct. 11, 2010, 5:49 p.m.
Hi,
this patch fixes an ICE (seen on ARM) in the ifcvt pass. The tail 
merging code in cond_exec_process_if_block(), under certain conditions, 
runs out into the previous basic block, causing an assertion fail later.

This is due to the use of next/prev_active_insn(), which does not check 
BB boundaries, and also skips use/clobber insns during post-reload. In 
the particular testcase I have, observed on ARM, prev_active_insn() is 
called on an insn which is preceded in that BB only by note and clobber 
insns, and ended up returning the jump insn in the preceding block...

So this patch is quite simple: implement two replacement functions 
find_active_insn_before/after() that correct the above mentioned 
behavior, and use them in ifcvt.

I'll also note that the included testcase triggers the same assert fail 
on ia64 too, and is also fixed by this patch, though I have not further 
tested it on that target.

Tested without new regressions on ARM-Linux using QEMU.
Ok for trunk?

Thanks,
Chung-Lin

2010-10-11  Chung-Lin Tang  <cltang@codesourcery.com>

         * ifcvt.c (find_active_insn_before): New function.
         (find_active_insn_after): New function.
         (cond_exec_process_if_block): Use new functions to replace
         prev_active_insn() and next_active_insn().


	testsuite/
	* gcc.dg/20101010-1.c: New testcase.
Eric Botcazou - Oct. 13, 2010, 7:51 a.m.
> So this patch is quite simple: implement two replacement functions
> find_active_insn_before/after() that correct the above mentioned
> behavior, and use them in ifcvt.

OK on principle, but the implementation isn't fully correct because it uses 
BLOCK_FOR_INSN and this isn't defined for BARRIERs.

Please add a BB parameter to both functions and test against BB_HEAD or BB_END 
to stop the iteration, like in first_active_insn/last_active_insn.

Patch

Index: ifcvt.c
===================================================================
--- ifcvt.c	(revision 165262)
+++ ifcvt.c	(working copy)
@@ -88,6 +88,8 @@ 
 static bool cheap_bb_rtx_cost_p (const_basic_block, int);
 static rtx first_active_insn (basic_block);
 static rtx last_active_insn (basic_block, int);
+static rtx find_active_insn_before (rtx);
+static rtx find_active_insn_after (rtx);
 static basic_block block_fallthru (basic_block);
 static int cond_exec_process_insns (ce_if_block_t *, rtx, rtx, rtx, rtx, int);
 static rtx cond_exec_get_condition (rtx);
@@ -229,6 +231,52 @@ 
   return insn;
 }
 
+static rtx
+find_active_insn_before (rtx insn)
+{
+  basic_block curr_bb;
+
+  if (!insn)
+    return NULL_RTX;
+
+  curr_bb = BLOCK_FOR_INSN (insn);
+
+  while ((insn = PREV_INSN (insn)) != NULL_RTX)
+    {
+      /* No other active insn all the way to the start of the basic block. */
+      if (curr_bb != BLOCK_FOR_INSN (insn))
+        return NULL_RTX;
+
+      if (NONJUMP_INSN_P (insn) || JUMP_P (insn) || CALL_P (insn))
+        break;
+    }
+
+  return insn;
+}
+
+static rtx
+find_active_insn_after (rtx insn)
+{
+  basic_block curr_bb;
+
+  if (!insn)
+    return NULL_RTX;
+
+  curr_bb = BLOCK_FOR_INSN (insn);
+
+  while ((insn = NEXT_INSN (insn)) != NULL_RTX)
+    {
+      /* No other active insn all the way to the end of the basic block. */
+      if (curr_bb != BLOCK_FOR_INSN (insn))
+        return NULL_RTX;
+
+      if (NONJUMP_INSN_P (insn) || JUMP_P (insn) || CALL_P (insn))
+        break;
+    }
+
+  return insn;
+}
+
 /* Return the basic block reached by falling though the basic block BB.  */
 
 static basic_block
@@ -447,9 +495,9 @@ 
       if (n_matching > 0)
 	{
 	  if (then_end)
-	    then_end = prev_active_insn (then_first_tail);
+	    then_end = find_active_insn_before (then_first_tail);
 	  if (else_end)
-	    else_end = prev_active_insn (else_first_tail);
+	    else_end = find_active_insn_before (else_first_tail);
 	  n_insns -= 2 * n_matching;
 	}
 
@@ -487,9 +535,9 @@ 
 	  if (n_matching > 0)
 	    {
 	      if (then_start)
-		then_start = next_active_insn (then_last_head);
+		then_start = find_active_insn_after (then_last_head);
 	      if (else_start)
-		else_start = next_active_insn (else_last_head);
+		else_start = find_active_insn_after (else_last_head);
 	      n_insns -= 2 * n_matching;
 	    }
 	}
@@ -645,7 +693,7 @@ 
     {
       rtx from = then_first_tail;
       if (!INSN_P (from))
-	from = next_active_insn (from);
+	from = find_active_insn_after (from);
       delete_insn_chain (from, BB_END (then_bb), false);
     }
   if (else_last_head)
Index: testsuite/gcc.dg/20101010-1.c
===================================================================
--- testsuite/gcc.dg/20101010-1.c	(revision 0)
+++ testsuite/gcc.dg/20101010-1.c	(revision 0)
@@ -0,0 +1,14 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-crossjumping" } */
+
+int foo (void)
+{
+  int len;
+  if (bar1 (&len))
+    {
+      char devpath [len];
+      if (bar2 (devpath) == len)
+        return len;
+    }
+  return -1;
+}